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

Remove the byteorder dependency. #109

Closed
wants to merge 1 commit into from
Closed

Remove the byteorder dependency. #109

wants to merge 1 commit into from

Conversation

RazrFalcon
Copy link

I understand that this is a controversial change and you do not want any unsafe in your library. But there are no benefits in "hiding" an unsafe in byteorder. So by using the same code directly, we will not make the base64 less safe.

On the bright side, we are reducing the build time by 2.5x. Which is very important for a such widely used library.

# before
> hyperfine 'cargo clean; cargo build --release'
  Time (mean ± σ):      1.729 s ±  0.006 s    [User: 2.570 s, System: 0.137 s]
  Range (min … max):    1.718 s …  1.737 s    10 runs
# after
> hyperfine 'cargo clean; cargo build --release'
  Time (mean ± σ):     657.2 ms ±  15.8 ms    [User: 861.8 ms, System: 62.5 ms]
  Range (min … max):   644.8 ms … 697.0 ms    10 runs

@gaetanww
Copy link

How about using std functions to avoid both unsafe and compile time?
from_be_bytes
to_be_bytes

@RazrFalcon
Copy link
Author

@gaetanww Nice, I've missed this methods. But they are available since 1.32 and this crate is target 1.27. So maybe it will not the best solution for the author.

@marshallpierce
Copy link
Owner

  • I don't follow why it's beneficial to have our own copy of this code rather than using the tested ones in byteorder.
  • This lib now uses 1.31, so 1.32 isn't far off, but even if we did use the std lib functions, those are just using unsafe under the hood too...
  • I'm curios what your workflow is that you're building base64 (and byteorder) repeatedly. In my experience, cargo does a good job of not rebuilding dependencies.

@RazrFalcon
Copy link
Author

I simply prefer dependencies without dependencies. Especially when they are not adding much.

We can strip a bit here, and a bit there and in the end our app builds like 2x faster. For me, it's an obvious improvement.

@gaetanww
Copy link

It's all about tradeoffs:

  • depending on byteorder increases compile time but pushes some maintenance
    burden to them.

  • relying on std functions pushes maintenance to the std maintainers, but
    prevents you from going no_std.

  • Calling an unsafe method is not unsafe if you ensure that some properties
    are always verified (like in the PR). On the other hand, some people like
    forbid_unsafe libraries because they're easier to audit.

I don't think there is a correct answer, just a matter of priorities
between. And you might also decide that it's a bike shed argument, which is
fine :)

@marshallpierce
Copy link
Owner

If moving from widely used and tested code to a one-off copy of the code sans tests that will not get any future patches, etc, seems like a good tradeoff for a library with an emphasis on correctness and security all in order to save 1s of one-time compile time, then we are unlikely to agree on much.

Very little rust is going to be completely unsafe free in practice (anything that uses the stdlib will fail that requirement). While that's a nice goal in theory, this change is overall a step backwards in reliability. You might as well inline a copy of all of the source of the stdlib and then say "look ma, no third party libraries" -- an approach which would have missed out on, for instance, on the fixes in 1.29.1. If an unsafe-free future is appealing (which I hope it is to everyone), consider putting your effort into rust-lang/rfcs#2610 instead.

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