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

Feature/improved bitstream efficiency #66

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SiebrenW
Copy link
Contributor

In this PR I'm improving the efficiency of the bitstream read/write by writing more than one bit at a time, if possible.
With a typical stream this makes the reading/writing ~2x faster (mostly due to the single bit writes being pretty much the same, strings, numbers and byte streams are typically 4x faster by my limited tests)
To test it was still working I employed this snippet: https://gist.github.com/SirTates/2527df0648e3dda3ae3cc27f0215ab85

@SiebrenW SiebrenW force-pushed the feature/improved_bitstream_efficiency branch from 155e547 to 766773c Compare January 18, 2024 21:07
@barsnick
Copy link
Contributor

barsnick commented Feb 2, 2024

Hi @SirTates, thanks for this contribution.

At first, we had doubts regarding performance. While it's obviously more efficient to mask complete (parts of) bytes into and out of a stream, instead of bit-by-bit, the most common operations in EXI are single-bit based. Nevertheless, a performance evaluation over our test base indeed confirmed your measurements: A two to fourfold improvement when writing streams, but no improvement (also no degradation) when reading. (Or was it vice versa, @chausGit?)

The are two big BUTs:

  1. Pretty much exactly 1/8th of our test streams gets an incorrect size with your code: One additional byte. This is because you don't understand the handling of the bit and byte counters correctly. This is also probably why you are missing a flush function (as you mentioned here, which is not required in cbV2G. 1 in 8 being incorrect likely means you got the counting wrong on bit overflow in a byte.
  2. Your inserted code looks very much like code from a different project, which is not acceptable. Our aim was to rewrite these low-level bitstream operations from scratch. That's also why our bit/byte counting is somewhat different. So we suggest you do the same when converting from single-bit streaming to multiple-bit masking: Rewrite from scratch. The masking approach is generally good, as mentioned above.

@SiebrenW
Copy link
Contributor Author

SiebrenW commented Feb 2, 2024

  1. I will look into that. this was indeed a bit less clear to me how it's exactly meant to work
  2. I apologise, I did use another project as reference for one of the functions (not the other), but did not copy a single line of code, I wrote it myself. I can redo this without reference to this code or the other reference (and now that I compare it again I agree. It didn't occur to me how similar it was).

Signed-off-by: Siebren Weertman <[email protected]>

improve bitstream read efficiency

Signed-off-by: Siebren Weertman <[email protected]>

rework
@SiebrenW SiebrenW force-pushed the feature/improved_bitstream_efficiency branch from 766773c to 27aa0cc Compare February 5, 2024 08:11
@SiebrenW
Copy link
Contributor Author

SiebrenW commented Feb 5, 2024

I rewrote the functions mostly from scratch (those bit manipulation parts have only few other ways to do the same, so I kept those, because that's what I spent most time on last time)
It should also have less branching and levels of indirection now, so it may be slightly faster now, though I have not tested that yet.

I rebased all in one commit to remove reference of the potentially offending code.

And I'm not sure whether I got the bit counting right this time. All my test streams go well and the length seems to not have changed from before and after my changes, but I couldn't replicate the same issue with the bit counting.

If I knew what combination of wite_bits caused it I could add that test.

@SebaLukas
Copy link
Collaborator

What is the status here? @barsnick I think it's also okay if we don't accept PRs.

@SiebrenW
Copy link
Contributor Author

SiebrenW commented Apr 16, 2024

I still think it's worth having. It significantly improves encode/decode speed.
Maybe someone can have a go who has not been tainted by existing examples, because I cannot help but make it look similar, even without reference.
I think there simply aren't that many ways to program it.

Though apparently there still are some issues:

  • It needs a small change to remove the unneeded static functions that are no longer required (read/write single bit).
  • There was a 1/8 case where the length would be calculated incorrectly. I'm not sure whether I fixed that, because I don't have the test cases

If the latter is no longer an issue and you still want the change, then I can fix the former issue too.

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.

3 participants