-
Notifications
You must be signed in to change notification settings - Fork 115
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
Stop using unsafe code #29
Comments
It's hard to be too up in arms about performance when we just had a security issue. :( I did see significant performance regressions without |
I too would like to encourage you to stop using unsafe code. Even if performance suffers, most projects using base64 are probably not bottle-necked on base64 encoding. Therefore, a general-purpose base64 crate (which the crate name implies this crate is) should choose safety over maximum possible performance. If a project finds that they are bottle-necked on base64 encoding and they have to speed it up at any cost, then they can use a special-purpose implementation that uses |
I had a play with removing the unsafe parts to see how the performance changed. First, a naive try:
That seems like a steep drop. An improved try:
I guess the remaining question is whether this crate has hard performance targets in the 10Gbps range. |
Here's another approach with a smaller performance hit:
This one is based off the #27. That PR introduces other usages of unsafe that apply only to line wrapping, but the restructuring of the encode path made it a little easier to work for this. It also included a reworked |
I wonder if further optimizations would be possible by limiting the customization of the character set such that |
Expanding on my previous comment: A few crypto-specific base64 implementations I've seen recently (e.g. BoringSSL) switched to a constant-time implementation that doesn't use table lookups at all. I imagine eliminating all table lookups entirely could improve performance, but it would be impractical unless the customization were strictly limited. |
Actually, the table lookups (according to perf and friends) seem to be really cheap -- the slowdown here seems to be due to bounds checking when writing to the output slice. :/ |
Side note -- writing into the output slice directly, rather than packaging bytes into a |
You might find this useful then: https://users.rust-lang.org/t/how-to-zip-two-slices-efficiently/2048. Note that the suggestions to use |
Also, maybe some of the tricks used to implement |
Thanks for the link; I ended up using a similar approach with some hand unrolling to get pretty close:
|
line wrap is now also unsafe-free in the |
@marshallpierce Internally, the Regardless, I think it is worthwhile to try to do everything without |
An aside from someone who has been intending to use this library: There are plenty of cases (e.g. storing binary data in JSON) where a base64 parser can be a performance bottleneck. So I think near-maximum speed is an important goal; not more important than correctness, of course, but in principle if a core functionality like this cannot be made fast enough without unsafe, then the burden of demonstrating correctness should be moved from the compiler to tests. (It looks as though the "fast enough" strategies have already been found, though.) It's easy to import instincts from other languages, "Oh, it's probably not the bottleneck", but I think this is less true in Rust because if performance truly wasn't the bottleneck anywhere in their code, people probably wouldn't be using Rust in the first place. |
Here are the options I see. One approach would be to switch the lower level encoding logic to just One way to continue to encode into a
That's not counting the cost of copying the bytes into the Another approach would be to post-hoc
I could maybe groan and tolerate that if that wasn't pretty much a best-case scenario: I have a fast Xeon with tons of cache and memory bandwidth and a clever prefetcher. On a lesser system (which I don't have easy access to), it may be significantly worse. If anyone has interesting systems they want to try this on, let me know. Furthermore, I'm not really sure how valuable this runtime check is. We still have the unsafe |
Sounds like setting up "cargo fuzz" for this crate, and having it run for a long time, would be useful. Regardless of how much unsafe'ness of the implementation can be prevented or not. See #21 |
See #26. :) |
fixed with #31 |
A base64 implementation in Rust shouldn't need to use
unsafe
for performance. The recent buffer overflow wouldn't have occurred if not for the use ofunsafe
.Even if we were not able to get the same level of performance with 100% safe code (I'm almost certain we can), I think most if not all users would trade a little performance for increased safety.
The text was updated successfully, but these errors were encountered: