Skip to content
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

Optimize and simplify BitReaderReversed #81

Merged
merged 4 commits into from
Dec 27, 2024

Conversation

fintelia
Copy link
Contributor

This PR refactors the BitReaderReversed object based on the structure from the Reading bits in far too many ways blog series.

It retains the get_bits method to read bits from the stream, but also exposes lower-level peek_bits, consume, and refill methods. These new methods may unlock optimizations in other parts of the code by reducing the number of hard-to-predict branches on whether to refill the bit container. And a separate peek_bits method might be especially useful for Huffman decoding because it lets you do a table lookup with N bits and then decide to consume fewer than N of them, rather than the current strategy of intentionally reading past the end of the input.

On my machine (a Ryzen 5600X), I see a ~5% end-to-end performance improvement in decode time for enwik9 from this change.

@KillingSpark
Copy link
Owner

KillingSpark commented Dec 23, 2024

This is an interesting changeset. I can't reproduce the performance gains on a Ryzen 5 3500U (it performs basically the same) but think I see the potential for the optimizations you mention in other regions of the library.

I'll look into this more after the holidays :)

Edit: Just for the simplification this is probably worth pulling, even if it doesn't improve performance just yet.

@KillingSpark
Copy link
Owner

KillingSpark commented Dec 27, 2024

Yep looks good, thanks for the PR!

Edit: On my Ryzen 3800X that's approximately a 10% improvement!

@KillingSpark KillingSpark merged commit 42d1b3a into KillingSpark:master Dec 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants