-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Move building fieldNames set to class constructor #131722
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
Move building fieldNames set to class constructor #131722
Conversation
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Here are the results of the latest benchmark: Summary:
(Not sure how useful throughput is as a metric here, from what I understand for search queries the important metric is latency.) At first glance, this is not the across-the-board improvement I was expecting. While the results are promising for limit_500, chicken_1, chicken_2, and chicken_3, the results look much worse for chicken_3_with_where and chicken_4. I'm going to run the benchmark, collect a profile, and generate a flamegraph to double-check that we're not seeing as much time spent in (I tried to collect a profile during the last benchmark run, but it seems I collected it during the indexing step when we're actually concerned with the search steps, so I need to re-run and re-collect). |
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
…ck-loader-optimization
…ck-loader-optimization
…ck-loader-optimization

This patch moves building the set of fieldName prefixes in the
FallbackSyntheticSourceBlockLoaderto the constructor. This set does not change between invocations (since thefieldNameis final), so we can just do the work once when constructing the BlockLoader instead of per-document.Resolves #130887