search

BurntSushi / byteorder

Rust library for reading/writing numbers in big-endian and little-endian.
The Unlicense
984 stars 144 forks source link

Replace some unsafe with zerocopy #214

Open joshlf opened 2 months ago

joshlf commented 2 months ago

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

BurntSushi commented 2 months ago

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 https://github.com/rust-lang/regex/pull/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Okay, that's helpful context, thanks!