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

Replace some unsafe with zerocopy #214

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

Conversation

joshlf
Copy link

@joshlf joshlf commented Oct 4, 2024

There may be more that could be done with some deeper restructuring, but I figured I'd keep it simple to start with.

@BurntSushi
Copy link
Owner

I love zerocopy and what it's trying to do, and if this stuff were in std, I'd absolutely have no hesitation doing this (assuming we waited long enough to keep the MSRV somewhat conservative). But I think my view is generally unchanged from rust-lang/regex#1189. This isn't bringing in the proc macro, but it still makes cargo clean && cargo b -r be around 4x slower on my machine. byteorder is used in tons of places, so adding this dependency here will end up adding it to a lot of dependency trees.

I think this stance probably applies to most of my crates. One place where I'd differ is for opt-in support like serde so that types implement the relevant zerocopy traits. I'm not sure if any of my crates fall into that bucket though. (Maybe bstr?)

I think I'd actually probably rather folks decrease dependency on byteorder. I wonder how many folks are using byteorder when they could just be using std's APIs that were added long after byteorder existed. Hah. Of course, byteorder offers other things, so IDK to what extent folks can move off of it.

@joshlf
Copy link
Author

joshlf commented Oct 4, 2024

But I think my view is generally unchanged from rust-lang/regex#1189.

No worries, I totally understand! In putting up these PRs, I made sure I hadn't already put up PRs to the same repo in the past that got declined, but I forgot about other repos by the same author - sorry for the noise!

it still makes cargo clean && cargo b -r be around 4x slower on my machine.

Do you think you'd be more inclined to take the dependency if there were less of a slowdown? Not that this particular PR is super important to me, but it's good to get a sense of what the maintainers of important crates care about. E.g. I could see us putting functionality behind default-enabled feature flags so that users who care can disable everything and speed up compile times.

@BurntSushi
Copy link
Owner

That's a good question. I honestly don't know. No matter how much it decreases, it's still non-zero. And I do overall feel like the code in byteorder is relatively low-risk and it isn't changing much if at all. It's been nearly frozen for many years. So while the downsides could maybe be mitigated to an extent, the upside isn't huge in this case.

I think I'd also be more inclined to do it if a lot more people were using zerocopy. (I don't have a good sense of how often it appears in dependency trees.) But that's a chicken-and-egg problem...

@joshlf
Copy link
Author

joshlf commented Oct 4, 2024

Okay, that's helpful context, thanks!

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