logos icon indicating copy to clipboard operation
logos copied to clipboard

Disable unsafe code with a `forbid_unsafe` feature

Open davidkern opened this issue 6 months ago • 7 comments

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 a cfg 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

davidkern avatar Aug 19 '24 00:08 davidkern