-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ESQL: Add memory Accountancy to Blocks and Vectors #99235
Conversation
Pinging @elastic/es-ql (Team:QL) |
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
...k/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/BlockAccountingTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment about the static methods. Otherwise LGTM.
@@ -57,6 +61,16 @@ public DoubleBlock expand() { | |||
return new DoubleArrayBlock(values, end, firstValues, shiftNullsToExpandedPositions(), MvOrdering.UNORDERED); | |||
} | |||
|
|||
public static long ramBytesEstimated(double[] values, int[] firstValueIndexes, BitSet nullsMask) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it useful that this is public static
? I feel like it might be useful to pre-bump a breaker, but you'd want to do that before allocating the arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's still a little unclear if these need to be public or not. I felt them public for now. The idea (for a subsequent PR), since the various arrays can grow independently from each other, is to account for / circuit-break the array element sizes in the builder, then compute and add the actual difference when building the block. That way we can work off reasonable estimates until we build the actual block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine update branch |
This commit adds memory accountancy to Blocks and Vectors.
This change is just the first step along the way to eventually add circuit breaking - we first need to the ability to determine ram usage, both before and after construction. Generally, we can track primitive array allocation and growth in builders separately (either before or after allocation), then use the block
ramBytesUsed
when releasing (giving back to the breaker). Care will be needed when calculating the actual ram bytes used before allocation (if needed), since that will need to take object reference and array header sizes into account, but seems reasonable.relates #99173