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

Add lifetime for encoder trait and add allocation improvements based on that (OER) #370

Merged
merged 35 commits into from
Nov 26, 2024

Conversation

Nicceboy
Copy link
Contributor

If we add lifetime for Encoder trait, we can recursively share the same output buffer for new encoders with the help of AnyEncoder associated type, without using RefCell and Rc combination.

We can avoid the runtime performance of RefCell and can guarantee that mutable reborrow does not happen in compile time.

Currently only OER does this, but it applies for other codecs too, if they will be also optimized.

I was also planning on adding encode_buf method for OER to reuse the same buffer, since it is trivial after this change, and maybe we can add something like that for other codecs too in small steps.

@XAMPPRocky
Copy link
Collaborator

Currently only OER does this, but it applies for other codecs too, if they will be also optimized.

Can you make tracking issues for this and other optimisations that have so far only been implemented for OER? I feel like the other codecs have a lot of catch up to do.

@Nicceboy
Copy link
Contributor Author

Currently only OER does this, but it applies for other codecs too, if they will be also optimized.

Can you make tracking issues for this and other optimisations that have so far only been implemented for OER? I feel like the other codecs have a lot of catch up to do.

I can add some notes later. Currently the primary focus have been just on attempting to reduce the allocations for theoretical minimum.

@Nicceboy
Copy link
Contributor Author

This will be likely the last performance optimization for OER in a while, after this PR it is around 20-30% faster than asn1c.

@Nicceboy Nicceboy force-pushed the buffer-recursive branch 2 times, most recently from 7782d7d to 768d909 Compare November 25, 2024 08:10
@Nicceboy Nicceboy force-pushed the buffer-recursive branch 4 times, most recently from b02ed36 to 7e80b86 Compare November 25, 2024 12:09
@Nicceboy
Copy link
Contributor Author

I think this can be reviewed as well.
There are no easy optimizations left anymore. Those constraints could be improved further, but other than that there is branch prediction and unsafe left. Or maybe see if some in-house parser is faster than nom since in OER the use-case is very simple, and check the penalty of some BitView/BitSlice operations.

In most cases encoding operations uses only two buffers now, regardless of the type complexity, and all the dynamic data is located in there (excluding BigInt temporaly).
Decoding allocates only for types that require it.

There is encode_buf function for OER now.

image

@Nicceboy Nicceboy marked this pull request as ready for review November 25, 2024 12:26
@Nicceboy Nicceboy changed the title Add lifetime for encoder trait and add allocation improvements based on that Add lifetime for encoder trait and add allocation improvements based on that (OER) Nov 25, 2024
@Nicceboy
Copy link
Contributor Author

When looking back to the dudycz's non-extended integer test, encoding speed is now around 25 faster and decoding speed around 50 times faster

image

@Nicceboy
Copy link
Contributor Author

I still removed some BitSlice usages (preamble & extension header) since the difference was measurable:

image

@XAMPPRocky XAMPPRocky merged commit 274e021 into librasn:main Nov 26, 2024
65 checks passed
@XAMPPRocky
Copy link
Collaborator

Thank you for your PR!

@github-actions github-actions bot mentioned this pull request Nov 26, 2024
@Nicceboy Nicceboy deleted the buffer-recursive branch November 26, 2024 09:56
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