Skip to content
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

ES|QL: account for page overhead when calculating memory used by blocks #108347

Merged

Conversation

luigidellaquila
Copy link
Contributor

@luigidellaquila luigidellaquila commented May 7, 2024

ES|QL accounts for memory used by single blocks, but it doesn't account for the Page.
In particular, Page.blocks array can have a significant footprint when we have thousands of columns in the result set.

Accounting for memory used by pages is difficult at this stage, so for now we'll approximate it adding some overhead to each block, assuming that a block will typically be linked to a single page.

Fixes #108104

+ RamUsageEstimator.shallowSizeOfInstance(BooleanVectorBlock.class);
+ RamUsageEstimator.shallowSizeOfInstance(BooleanVectorBlock.class)
// TODO: remove this if/when we account for memory used by Pages
+ RamUsageEstimator.NUM_BYTES_OBJECT_ALIGNMENT;
Copy link
Contributor Author

@luigidellaquila luigidellaquila May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly, this should be NUM_BYTES_OBJECT_REF, but it would need further alignment to object size, so practically it would make no difference, to the cost of one more operation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure we should account for the refs this way, but it does seem like a reasonable solution, at least for a while.

I think we should make a constant in Block for this rather than use NUM_BYTES_OBJECT_ALIGNMENT - that one feels a little random. Or, if it is random, we can explain why it's the right value on the constant.

@luigidellaquila luigidellaquila marked this pull request as ready for review May 7, 2024 13:23
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 7, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks Luigi!

@luigidellaquila luigidellaquila merged commit c9b8d72 into elastic:main May 9, 2024
15 checks passed
markjhoy pushed a commit to markjhoy/elasticsearch that referenced this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ES|QL: HeapAttack.testTooManyEval failing on serverless
4 participants