-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
GH-36297: [C++][Parquet] Benchmark for non-binary dict encoding #36298
GH-36297: [C++][Parquet] Benchmark for non-binary dict encoding #36298
Conversation
|
@pitrou Would you mind take a look? Or here are already some benchmarks here? |
Why not, but is this useful? Is dictionary encoding often used with integers? |
Hmm sometimes we use dictionary with integer, and I found that there're not benchmark for it. So add this benchmark. |
Are there decoding benchmarks already? |
@pitrou Seems that we already have some decode test before. But they only test one value, see |
19bfd91
to
ac35a89
Compare
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.
Thanks @mapleFU ! A couple other suggestions.
encoder->FlushValues(); | ||
} | ||
|
||
state.SetBytesProcessed(state.iterations() * state.range(0) * sizeof(T)); |
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.
Can you also add a SetItemsProcessed
here and possibly in other benchmark functions?
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.
And by the way:
state.SetBytesProcessed(state.iterations() * state.range(0) * sizeof(T)); | |
state.SetBytesProcessed(state.iterations() * num_values * sizeof(T)); |
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 addressed only one comment here.
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.
Sorry for missing the message, I've fixed it and paste the result
Comment fixed |
M1 MacOS, clang++13.0, Release(O2):
|
df7c6ab
to
f233f75
Compare
Conbench analyzed the 6 benchmark runs on commit There were 8 benchmark results indicating a performance regression:
The full Conbench report has more details. |
Rationale for this change
Add benchmark for non-binary dict encoding
What changes are included in this PR?
Add benchmark
BM_DictEncodingInt64
Are these changes tested?
no need
Are there any user-facing changes?
no