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

Make two SubReaderWrapper implementations singletons #112596

Merged

Conversation

original-brownbear
Copy link
Member

random find:

No state in either of these, no need to recreate them for every directory. Saves a few cycles and makes it more obvious that there's no state in these.

No state in either of these, no need to recreate them for every
directory. Saves a few cycles and makes it more obvious that there's no
state in these.
@original-brownbear original-brownbear added >non-issue :Search Foundations/Search Catch all for Search Foundations labels Sep 6, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.16.0 labels Sep 6, 2024
Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM, left a minor renaming suggestion

final int numDocs = segmentReader.maxDoc() - segmentReader.getSegmentInfo().getDelCount();
assert numDocs == popCount(hardLiveDocs) : numDocs + " != " + popCount(hardLiveDocs);
return new LeafReaderWithLiveDocs(segmentReader, hardLiveDocs, numDocs);
private static final SubReaderWrapper SUB_READER_WRAPPER = new SubReaderWrapper() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static final SubReaderWrapper SUB_READER_WRAPPER = new SubReaderWrapper() {
private static final SubReaderWrapper ALL_LIVE_DOCS_SUB_READER_WRAPPER = new SubReaderWrapper() {

@original-brownbear original-brownbear added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 11, 2024
@original-brownbear
Copy link
Member Author

Thanks Tanguy!

@original-brownbear
Copy link
Member Author

@elasticmachine update branch

@elasticsearchmachine elasticsearchmachine merged commit 213322a into elastic:main Sep 11, 2024
15 checks passed
@original-brownbear original-brownbear deleted the static-sub-wrappers branch September 11, 2024 10:52
davidkyle pushed a commit that referenced this pull request Sep 12, 2024
random find:

No state in either of these, no need to recreate them for every
directory. Saves a few cycles and makes it more obvious that there's no
state in these.
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!) >non-issue :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants