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

Purpose of Box in StreamCDC struct #25

Closed
tazjin opened this issue Mar 10, 2023 · 2 comments
Closed

Purpose of Box in StreamCDC struct #25

tazjin opened this issue Mar 10, 2023 · 2 comments

Comments

@tazjin
Copy link

tazjin commented Mar 10, 2023

Hey!

The StreamCDC struct currently contains a Box<dyn Read>, which requires an additional heap allocation and potentially prevents some compiler optimisations by obscuring the reader type.

Is there a particular reason for this Box that isn't immediately obvious, or would you accept a change to parameterise StreamCDC over an R: Read, such that monomorphisation can occur?

Thanks for this crate! :)

@nlfiedler
Copy link
Owner

It was that way because I didn't know any better. In an effort to learn Rust more thoroughly I went ahead and made the change. Thank you for pointing it out, and offering to fix it.

@tazjin
Copy link
Author

tazjin commented Mar 10, 2023

Great, thank you! :)

tvlbot pushed a commit to tvlfyi/tvix that referenced this issue Mar 11, 2023
This removes the use of Box::new, switching fastcdc to version 3.0.2
with nlfiedler/fastcdc-rs#25 fixed.

Change-Id: I64f388b9e0a7f358e25a8bb7ca0e4df1d3bb01c4
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8249
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: flokli <flokli@flokli.de>
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

No branches or pull requests

2 participants