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

Reduce memory usage of match all bitset #92777

Merged
merged 3 commits into from
Jan 11, 2023

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jan 10, 2023

By default, Elasticsearch uses up to 1 bit per document to store the set of root documents when the index mapping has nested fields. This PR introduces a special BitSet using less memory for match_all filters. This optimization is only triggered when the index mapping has a nested field, but that field never exists in documents.

@dnhatn dnhatn changed the title Reduce memory usage of match_all BitSet Reduce memory usage of match all bitset Jan 10, 2023
@dnhatn dnhatn closed this Jan 10, 2023
@dnhatn dnhatn removed the v8.7.0 label Jan 10, 2023
@dnhatn dnhatn deleted the match_all_bits branch January 10, 2023 05:19
@dnhatn dnhatn restored the match_all_bits branch January 11, 2023 00:04
@dnhatn dnhatn reopened this Jan 11, 2023
@dnhatn dnhatn marked this pull request as ready for review January 11, 2023 00:09
@dnhatn dnhatn added v8.7.0 >enhancement :Search/Search Search-related issues that do not fall into other categories labels Jan 11, 2023
@dnhatn dnhatn requested a review from jpountz January 11, 2023 00:10
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@elasticsearchmachine
Copy link
Collaborator

Hi @dnhatn, I've created a changelog YAML for you.

@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Jan 11, 2023
if (bits == null) {
initializeBitSet();
}
bits.clear(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need to support clearing bits on this MatchAllBitSet? It's counter intuitive to me that something called MatchAllBitSet could have some of its bits not set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I've pushed 0f12598 to make it read-only.

@dnhatn dnhatn requested a review from jpountz January 11, 2023 15:16
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

This looks fine to me. For the record, I'm in general not a fan of implementing optimizations for cases that should be as rare as this one, but this concern is alleviated by the fact that it shares logic with the same optimization for DLS where I would expect this condition to be less rare.

@dnhatn
Copy link
Member Author

dnhatn commented Jan 11, 2023

Thanks, Adrien. I was also hesitant to open this PR, but this class already exists in the security module.

@dnhatn dnhatn added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jan 11, 2023
@elasticsearchmachine elasticsearchmachine merged commit fa58477 into elastic:main Jan 11, 2023
@dnhatn dnhatn deleted the match_all_bits branch January 11, 2023 15:49
danielmitterdorfer pushed a commit to danielmitterdorfer/elasticsearch that referenced this pull request Jan 12, 2023
By default, Elasticsearch uses up to 1 bit per document to store the set
of root documents when the index mapping has nested fields. This PR
introduces a special BitSet using less memory for match_all filters.
This optimization is only triggered when the index mapping has a nested
field, but that field never exists in documents.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >enhancement :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants