Avoid cycling searchers & leafs in ReservoirSampler; prefer source lookups #15078
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…okups So far the sampling was done in two steps: 1. Sample docIds 2. Load values for docIds The sampled docIds could hit many different readers and they weren't processed in order, which could multiply the number of times a reader needed to be loaded, and was taxing the fs cache. Further, this switches to use source lookups, since on large tables only a small subset of values is actually read. It avoids loading doc-values, and should interact better with the rate-limiting setting.
BaurzhanSakhariev
approved these changes
Nov 21, 2023
|
I took some profiles using JFR with a custom profile that captures all disk events Old version: New Version: So it does look like an improvement to me. |
mfussenegger
added a commit
that referenced
this pull request
Nov 22, 2023
Subset of #15078 I couldn't verify if the segment access optimization has any impact I also couldn't get realiable outputs with JFR FileRead sampling anymore. So, this instead shows the dstat output, recorded while the analyze was running. The first column is the status-quo, the second column is _this_ commit, the third column is the linked PR. -dsk/total- -dsk/total- -dsk/total- read writ read writ read writ 38M 45M 39M 45M 37M 45M 0 664k 0 272k 0 0 0 0 6584k 63M 0 0 0 0 1016k 59M 0 584k 7056k 102M 16k 0 0 24k 0 0 0 0 0 0 16k 0 0 0 0 232k 68M 0 0 0 0 0 1317M 0 126M 1056k 16k 8488k 4250M 512k 2024M 0 0 0 3748M 584k 1787M 0 0 0 3818M 136k 1605M 0 6744k 6272k 3819M 0 1497M 0 0 1488k 3872M 392k 1248M 912k 16k 2024k 3790M 0 1095M 0 91M 0 3778M 584k 946M 0 587M 0 3710M 0 807M 0 1562M 504k 3650M 0 729M 0 1593M 0 3130M 0 419M 136k 1370M 0 2619M 0 120M 11M 1039M 0 2223M 584k 0 664k 929M 0 1715M 0 0 1328k 599M 0 678M 0 2914M 0 118M 2312k 1088M 1032k 0 0 9008k 26M 0 584k 0 0 0 0
mfussenegger
added a commit
that referenced
this pull request
Nov 27, 2023
Subset of #15078 I couldn't verify if the segment access optimization has any impact I also couldn't get realiable outputs with JFR FileRead sampling anymore. So, this instead shows the dstat output, recorded while the analyze was running. The first column is the status-quo, the second column is _this_ commit, the third column is the linked PR. -dsk/total- -dsk/total- -dsk/total- read writ read writ read writ 38M 45M 39M 45M 37M 45M 0 664k 0 272k 0 0 0 0 6584k 63M 0 0 0 0 1016k 59M 0 584k 7056k 102M 16k 0 0 24k 0 0 0 0 0 0 16k 0 0 0 0 232k 68M 0 0 0 0 0 1317M 0 126M 1056k 16k 8488k 4250M 512k 2024M 0 0 0 3748M 584k 1787M 0 0 0 3818M 136k 1605M 0 6744k 6272k 3819M 0 1497M 0 0 1488k 3872M 392k 1248M 912k 16k 2024k 3790M 0 1095M 0 91M 0 3778M 584k 946M 0 587M 0 3710M 0 807M 0 1562M 504k 3650M 0 729M 0 1593M 0 3130M 0 419M 136k 1370M 0 2619M 0 120M 11M 1039M 0 2223M 584k 0 664k 929M 0 1715M 0 0 1328k 599M 0 678M 0 2914M 0 118M 2312k 1088M 1032k 0 0 9008k 26M 0 584k 0 0 0 0
|
Closing this in favour of #15087 |
mergify bot
pushed a commit
that referenced
this pull request
Nov 27, 2023
Subset of #15078 I couldn't verify if the segment access optimization has any impact I also couldn't get realiable outputs with JFR FileRead sampling anymore. So, this instead shows the dstat output, recorded while the analyze was running. The first column is the status-quo, the second column is _this_ commit, the third column is the linked PR. -dsk/total- -dsk/total- -dsk/total- read writ read writ read writ 38M 45M 39M 45M 37M 45M 0 664k 0 272k 0 0 0 0 6584k 63M 0 0 0 0 1016k 59M 0 584k 7056k 102M 16k 0 0 24k 0 0 0 0 0 0 16k 0 0 0 0 232k 68M 0 0 0 0 0 1317M 0 126M 1056k 16k 8488k 4250M 512k 2024M 0 0 0 3748M 584k 1787M 0 0 0 3818M 136k 1605M 0 6744k 6272k 3819M 0 1497M 0 0 1488k 3872M 392k 1248M 912k 16k 2024k 3790M 0 1095M 0 91M 0 3778M 584k 946M 0 587M 0 3710M 0 807M 0 1562M 504k 3650M 0 729M 0 1593M 0 3130M 0 419M 136k 1370M 0 2619M 0 120M 11M 1039M 0 2223M 584k 0 664k 929M 0 1715M 0 0 1328k 599M 0 678M 0 2914M 0 118M 2312k 1088M 1032k 0 0 9008k 26M 0 584k 0 0 0 0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So far the sampling was done in two steps:
The sampled docIds could hit many different readers and they weren't
processed in order, which could multiply the number of times a reader
needed to be loaded, and was taxing the fs cache.
Further, this switches to use source lookups, since on large tables only
a small subset of values is actually read. It avoids loading doc-values,
and should interact better with the rate-limiting setting.