-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Small optimization in Parquet varint decoder #8742
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
|
bench on intel i7-12700K |
|
Hmm...github lost a comment last night. bench on intel macbook |
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.
Makes sense to me. Thank you @etseidl -- I queued up a benchmark to confirm
Another thing we could try if we wanted to get all crazy is manually unrolling the loop (at least for the first 4 or 8 bytes) to remove the back branch 🤔
|
BTW it was nice to read https://en.wikipedia.org/wiki/Variable-length_quantity understand this better |
This is a great observation btw |
There's actually prior art in the rust compiler. rust-lang/rust#92604 |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
The benchmark results look consistent with an improvment to me -- great work @etseidl |
I got crazy and gave it a try: |
Which issue does this PR close?
Rationale for this change
Following the recent improvements in Thrift decoding, the percentage of time spent decoding LEB128 encoded integers has increased.
What changes are included in this PR?
This PR modifies the varint decoder to first test for integers that can be encoded in a single byte (using zig-zag encoding, the maximum int that can be encoded is 63). Many of the fields in the Parquet footer (including all enum values) will be in this range, so optimizing for this frequent occurrence makes sense.
Are these changes tested?
Should be covered by existing tests
Are there any user-facing changes?
No