Skip to content

Zero-Copy Slice Readers #530

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

krakow10
Copy link
Contributor

@krakow10 krakow10 commented May 5, 2025

This splits off chunks of a byte slice instead of allocating a String or Vec where applicable. There are several different classes of changes here that are interlaced in the diff, but I have grouped similar changes by commit to make it easier to review the repetitive changes. Potentially 5-8% faster on the deserialize microbenchmarks, but I can't really trust my benchmarks.

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a clever boy and can work out that the new functions aren't a part of RbxReadExt because they operate explicitly on a slice instead of a nebulous impl Read, but it might be worth writing it down for future generations. I'll leave that to your discretion, but I tend to err on the side of writing things down even if they feel obvious because it lowers the bus factor for the project.

Otherwise this seems fine to me.

@krakow10
Copy link
Contributor Author

krakow10 commented May 13, 2025

the new functions aren't a part of RbxReadExt ... it might be worth writing it down

Done. When I was writing this PR at first I was considering a trait too, but decided this was simpler. If you think you can improve the clarity of the documentation wording, suggestions or changes are welcome.

@krakow10 krakow10 mentioned this pull request May 20, 2025
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