Add benchmarks for dictionary path of new_group_values#22004
Open
Rich-T-kid wants to merge 2 commits intoapache:mainfrom
Open
Add benchmarks for dictionary path of new_group_values#22004Rich-T-kid wants to merge 2 commits intoapache:mainfrom
Rich-T-kid wants to merge 2 commits intoapache:mainfrom
Conversation
kumarUjjawal
reviewed
May 4, 2026
|
|
||
| for &size in &SIZES { | ||
| let mut cards = CARDS_RELATIVE.to_vec(); | ||
| cards.push(size); // all-unique stress case |
Contributor
There was a problem hiding this comment.
For size == 8192, this adds 8192 again because CARDS_RELATIVE already has 8 * 1024. Criterion needs unique benchmark IDs, so this benchmark panics before it can run. Please dedupe cards or remove 8 * 1024 from CARDS_RELATIVE.
Contributor
Author
There was a problem hiding this comment.
replaced 8*1024 with 1000
| //! column. Each iteration is timed end-to-end: it constructs the | ||
| //! `Box<dyn GroupValues>` returned by `new_group_values`, runs `intern` | ||
| //! once (or N times), and then `emit(EmitTo::All)`. | ||
|
|
Contributor
There was a problem hiding this comment.
This says GroupValues construction is timed, but new_group_values is inside the iter_batched_ref setup closure, so Criterion does not measure it, please update the comment to say the measured part is intern + emit.
Contributor
Author
|
@kumarUjjawal Just updated it, let me know if theres anything else I should change |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Which issue does this PR close?
benchmarks for #21765. Also related to #21860
The goal is to merge this PR and then rebase the branch on #21765 to contain these benchmarks, so that they can be run and compared to the original.
Rationale for this change
Originally this was included in #21765 but that PR is already very large. I decided to move it to its own separate PR
What changes are included in this PR?
Adds benchmarks for the dictionary encoding array path of new_group_values().
Are these changes tested?
n/a
Are there any user-facing changes?
no