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.

Sign up for GitHub

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

Jump to bottom

Speedup FieldsVisitor.reset #122990

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Speedup FieldsVisitor.reset #122990

wants to merge 1 commit into from

Conversation

original-brownbear
Copy link
Member

Random find from profiling. addAll is needlessly costly here (see profile below, it's like 1/4 of advancing the stored fields loader), seems even though the method is quite hot the creation of the iterator over the constant set doesn't compile away.
-> just unroll to 2 adds and do the same in the only remaining use-site of the set

image

@original-brownbear
Speedup FieldsVisitor.reset
1cecb11 
Random find from profiling. `addAll` is needlessly costly here,
seems even though the method is quite hot the creation of the iterator
over the constant set doesn't compile away.
-> just unroll to 2 `add`s and do the same in the only remaining
use-site of the set
@original-brownbear original-brownbear added >non-issue :Search Foundations/Search Catch all for Search Foundations labels Feb 20, 2025
@elasticsearchmachine elasticsearchmachine added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v9.1.0 labels Feb 20, 2025
@elasticsearchmachine
Copy link
Collaborator

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

javanna
javanna reviewed Feb 21, 2025
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

I honestly don't know about this one. The code around stored fields loading is all but easy to follow, and at least that least makes it clear that we remove and add the same set of fields.

@original-brownbear
Copy link
Member Author

@javanna I could make it an array for these fields and it should come out the same after the compiler is done with it :)
I was just thinking that inlining was fewer LoC and having a constant technically mutable array didn't feel great.
But worst case, I think just using a List.of might also work too, didn't benchmark it though.
As far as performance goes this is def. the winner :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javanna javanna Awaiting requested review from javanna

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
>non-issue :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@original-brownbear @elasticsearchmachine @javanna