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

Use set-len when reading into buffer #471

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

alexheretic
Copy link
Contributor

Minor optimisation of read buffer logic using set_len to avoid writing zeroes, more worthwhile if configuring large read buffer sizes.

Also simplified the code a bit using the buffer directly instead of split_off + unsplit.

@daniel-abramov
Copy link
Member

Thanks for all performance-related improvements, @alexheretic! Once this one is merged, we'll be good to go with the release, right? (you don't plan further optimizations for now, do you? 🙂 )

@daniel-abramov daniel-abramov merged commit 441f29f into snapview:master Dec 17, 2024
7 checks passed
@alexheretic alexheretic deleted the read-set-len branch December 17, 2024 17:38
@alexheretic
Copy link
Contributor Author

Thanks for all performance-related improvements, @alexheretic! Once this one is merged, we'll be good to go with the release, right? (you don't plan further optimizations for now, do you? 🙂 )

I don't have anything queued up no 😅 so if you're happy with all the API changes then a new release is fine by me 👍

Copy link

@conradludgate conradludgate 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 pretty sure this is unsound. A dodgy but valid Read::read impl is able to read the uninitialised bytes.

@alexheretic
Copy link
Contributor Author

I'm pretty sure this is unsound. A dodgy but valid Read::read impl is able to read the uninitialised bytes.

Ok thanks, on re-reading Read docs it does indeed clarify this. I'll raise a pr changing this back to zero filling then.

@daniel-abramov
Copy link
Member

daniel-abramov commented Dec 19, 2024

Great point! I also have not thought about that until now.

Implementations of this method can make no assumptions about the contents of buf when this function is called. It is recommended that implementations only write data to buf instead of reading its contents.

But at the same time it explicitly says:

Correspondingly, however, callers of this method in unsafe code must not assume any guarantees about how the implementation uses buf. The trait is safe to implement, so it is possible that the code that’s supposed to write to the buffer might also read from it. It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit) is not safe, and can lead to undefined behavior.


@alexheretic, I assume I can release a new patch version with just reverted change (the previous implementation was sound and not much slower anyway as you mentioned)

UPD: published 0.26.1.

@alexheretic
Copy link
Contributor Author

alexheretic commented Dec 19, 2024

I'd rather remove just the set_len usage than revert the whole pr... but I guess it isn't a big deal

@daniel-abramov
Copy link
Member

Makes sense!

NB: I've already published 0.26.1 with the commit reverted (I just immediately followed my reflexes and reverted the commit "just to be on the safe side" right away so that we have a safe published version before someone starts actively using 0.26.0)

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