logos
logos copied to clipboard
Disable unsafe code with a `forbid_unsafe` feature
See Issue: #398
As discussed in the linked issue, this PR adds an allow_unsafe
feature and gates all unsafe code on it.
This does not include that feature in the default features, so the effect of this PR would be to effectively remove unsafe code from logos unless it is opted-in.
There should be a discussion (on the issue) of the right strategy to roll this out. Removing unsafe
code immediately might not be the best option.
There is an additional issue with the Source
trait which this PR does not currently resolve.
Benchmarks
Throughput of the benches for this PR and the original code (in GiB/s). Method: bench was run on release build four times, alternating between toggling allow_unsafe
on and off. No extra effort was made to quiesce the machine other than just close other desktop apps.
It'd be very beneficial to get benchmarks on other CPUs, given the surprising result for the i7.
Mac M3 Air
Benchmark | Safe (this PR) | Unsafe (original) | Delta |
---|---|---|---|
iterate/identifiers | 1.6214 | 1.6862 | -3.84% |
iterate/strings | 1.2354 | 1.2479 | -1.00% |
iterate/keywords_operators_and_punctators | 2.0598 | 2.0721 | -0.59% |
count_ok/identifiers | 1.6191 | 1.6742 | -3.29% |
count_ok/keywords_operators_and_punctators | 1.2391 | 1.2423 | -0.25% |
count_ok/strings | 2.0746 | 2.0782 | -0.17% |
Disabling unsafe code does slow down throughput by a small amount, though not as much as I anticipated.
i7-7700HQ (Alienware 13 R3)
Benchmark | Safe (this PR) | Unsafe (original) | Delta |
---|---|---|---|
iterate/identifiers | 1.0751 | 0.9426 | +14.06% |
iterate/strings | 0.9987 | 0.9204 | +8.51% |
iterate/keywords_operators_and_punctators | 1.3997 | 1.2977 | +7.86% |
count_ok/identifiers | 1.0430 | 0.9526 | +9.49% |
count_ok/keywords_operators_and_punctators | 0.9497 | 0.9088 | +4.50% |
count_ok/strings | 1.3772 | 1.2962 | +6.25% |
I did not expect this result, at all. I suspect the compiler is able to take an optimization it otherwise couldn't, but I've not investigated why yet.
Next steps
- Some more benchmarks
- Decision on default state of feature flag or just remove unsafe
- Documentation
- Mark
Source
unsafe if unsafe code is enabled - I think I have to duplicate the entire trait under acfg
because you can't conditionally compile an effect (unsafe) afaik -
read_byte_unchecked
use is emitted by the code generator, will need to bring it back and also introduce a safe alternative