-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Use sequential access of stored fields in CCR #68961
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
Conversation
e7ae7e4
to
3d2dcde
Compare
3d2dcde
to
68053ef
Compare
Below is a benchmark from
|
Pinging @elastic/es-distributed (Team:Distributed) |
} | ||
assertFalse(snapshot.useSequentialStoredFieldsReader()); | ||
} | ||
// disable optimization for non-sequential accesses |
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.
This comment doesn't seem to correspond with the test below?
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.
Good catch. I fixed in 3eb853d.
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.
I left one comment, LGTM otherwise
} | ||
} | ||
|
||
private static boolean hasSequentialAccess(ScoreDoc[] scoreDocs) { |
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.
You can avoid the loop entirely like in FetchPhase#hasSequentialDocs
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.
This is a bit different. We can access documents out of order instead of ascending like in the fetch phase.
run elasticsearch-ci/2 |
@romseygeek @jimczi Thanks for review. |
This commit re-introduces the sequential access of stored fields in CCR. Unlike the previous change, we apply this optimization only when we are accessing 10+ consecutive document ids. I ran a CCR benchmark, and this change increased the indexing throughput on the leader by 30%.
We can't enable the sequential access optimization for stored fields of changes snapshots used in peer recoveries because they are accessed by multiple threads. Relates to #68961
We can't enable the sequential access optimization for stored fields of changes snapshots used in peer recoveries because they are accessed by multiple threads. Relates to elastic#68961
We can't enable the sequential access optimization for stored fields of changes snapshots used in peer recoveries because they are accessed by multiple threads. Relates to #68961
We can't enable the sequential access optimization for stored fields of changes snapshots used in peer recoveries because they are accessed by multiple threads. Relates to #68961
This commit re-introduces the sequential access of stored fields in CCR. Unlike the previous change, we apply this optimization only when we are accessing 10+ consecutive document ids.
I ran a CCR benchmark, and this change increased the indexing throughput on the leader by 30%.