An approach to unsafe optimisation

tutorials
1

I suspect we have all been tempted to experiment with unsafe optimisations, I recently thought I would have a go, and this is a report on how it went.

I decided to have a macro perf_assert! which defined as :

/// In debug mode or feature unsafe_opt not enabled, same as debug_assert! otherwise unsafe compiler hint.
#[cfg(any(debug_assertions, not(feature = "unsafe_opt")))]
macro_rules! perf_assert {
    ( $cond: expr ) => {
        debug_assert!($cond)
    };
}

/// In debug mode or feature unsafe_opt not enabled, same as debug_assert! otherwise unsafe compiler hint.
#[cfg(all(not(debug_assertions), feature = "unsafe_opt"))]
macro_rules! perf_assert {
    ( $cond: expr ) => {
        if !$cond {
            unsafe { std::hint::unreachable_unchecked() }
        }
    };
}

So this is controlled by a feature unsafe_opt ( disabled by default! ). I had to adjust the module header which forbids unsafe code, to allow the macro to be used:

#![cfg_attr(
    any(debug_assertions, not(feature = "unsafe_opt")),
    forbid(unsafe_code)
)] // see util::perf_assert! macro

Then I use the macro as follows in my Heap indexing code to "suppress" index checking:

        perf_assert!(x < self.0.len());
        &self.0[x]

( Full code here: heap.rs - source )

Finally I made a test with timing ( included in module above ) that shows about a 20% performance gain for heap operations, which actually surprised me a little as I didn't think it would make that much difference. Of course in a real system the time spent doing heap operations may well be negligible, that is harder to say. But at least I could measure something! My test output:

C:\Users\ano31\Rust\RustDB>benchheap

C:\Users\ano31\Rust\RustDB>cargo test heap --release -- --nocapture
    Finished release [optimized] target(s) in 0.13s
     Running unittests src\lib.rs (target\release\deps\rustdb-79526361b2f7e23f.exe)

running 2 tests
test heap::test ... ok
heap test average time=591 sorted results=[577, 578, 583, 584, 585, 591, 599, 603, 604, 608]
test heap::test2 ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.07s


C:\Users\ano31\Rust\RustDB>cargo test heap --release --features unsafe_opt -- --nocapture
    Finished release [optimized] target(s) in 0.05s
     Running unittests src\lib.rs (target\release\deps\rustdb-98231b58c6b06f4c.exe)

running 2 tests
test heap::test ... ok
heap test average time=510 sorted results=[480, 484, 504, 507, 517, 519, 522, 523, 524, 525]
test heap::test2 ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.06s


C:\Users\ano31\Rust\RustDB>
2

The main idea I wanted to suggest is crate authors should have a feature to enable/disable unsafe optimisations to give their clients the choice as to whether to opt into it or not.... I haven't seen this in crates I use so far. Any examples?

3

This would put the "burden of proof" on the caller to make sure it never ever, even potentially, could invoke undefined behavior. That would make the function that uses it unsound, unless it's either marked as unsafe or always checks the same thing earlier and diverts. The fact that it experimentally works in debug mode doesn't prove anything about the optimized version. The compiler can and will use it as proof for changing absolutely anything it can get away with.

It could perhaps be fine in an unsafe function, to check its preconditions, though. It would just have to be without the unsafe block it adds. Variants of that exists.

4

The "unsafety" is contained in the heap module ( I do have an assert! to check the index to the modify method to enforce the containment ). However if there is a logic error in the heap module of some kind, the use of unsafe could cause memory corruption etc. Library clients can either live with that possibility, or choose not to opt into it. Oh, well, VecU is actually public at the moment, it shouldn't be strictly speaking. It isn't meant to be part of the public interface.

5

The check outside the index function doesn't make it less unsound. If you move the exact same code to something like an unsafe fn get_unchecked, you would be fine in the sense that it correctly declares that its usage comes with some extra rules that can't be statically proven. This is how slices do it, for example.

6

Well I claim that GHeap is sound ( with unsafe_opt enabled ). I could try to give a mathematical proof, and there is also a test, although of course testing doesn't prove it is sound.

That is to say, whatever you do with GHeap, you will not get undefined behaviour, memory corruption. Unless I am wrong!

[ I made a cleaned up version which only publishes what ought to be public:
rustdb::heap - Rust ]

Edit: now I think about it, my check on the index to modify is not sufficient, I need to check it is both in range but also not a deallocated node index... or maybe not, I think if it is invalid but iun range, it still doesn't produce out-of-range indexes. ]

7

I hope that's true, but that doesn't matter. It's still a trap for you or anyone else. There's nothing wrong with using unsafe in an of itself. On the contrary, it's an important signal that one has to take extra care to get things right. Just like the type system or any other static check. It lets you assume that the rest of the program is correct.

8

Well using unsafe is always going to be a potential trap, a logic mistake will result in chaos, my point here is that by making it a non-default feature, you give the client a choice as to whether to opt into the potential chaos, or to trust that the module / crate has been correctly coded.

Another point is that even if a crate is memory safe, it doesn't mean it cannot do extremely bad things, like lose all your data!

9

Regular unsafe requires you to "unlock" its usage with an unsafe block or function. That's a conscious action at the site of usage, as opposed to something that doesn't look unsafe but may be if someone flipped the right switch. Except the compiler will not say anything. I'm still not sure if it would help that you checked everything. It's still breaking the rules and assumptions of the language.

10

Regular unsafe is a convention to say "you must satisfy certain conditions when calling this function or memory corruption may happen". That isn't the case here (once I tidied up the public interface that is), the danger is simply that my module code is wrong, as with all code that uses unsafe. That is flagged to some extent in that the crate user has to opt in to the potential unsafety ( this is something I just chose to do).

I could have just gone and used unsafe without any obvious warning. I feel crate authors who do that are perhaps being a little unreasonable. I also think perhaps Rust should prevent that, so crate users KNOW if they are using an unsafe crate. I am not quite sure how that could work though.

Perhaps a compiler flag that causes the compiler to warn about crates using unsafe would help ( together with some way to explicitly suppress the warning when specifying a dependency ).

11

It's a contract that is a fundamental part of what makes Rust what it is. We promise not to break it (bugs may happen, but should be fixed) and the compiler uses that assumption in various cases, like making better code and allowing us to not worry about it. unsafe is most importantly a safety fence around the axiomatic parts that the compiler cannot reason about (due to the halting problem or otherwise). You wouldn't have a useful standard library without this possibility, and that safety fence is kept as tight as possible. The contract says that if those parts are free from UB, then it follows that the usage of those functions will never invoke UB. If UB is still invoked, it's the fault of the function with the unsafe block.

Using it for optimization is more of a last resort, but it's under the same contract. It's sometimes possible to achieve the exact same thing with a simple assert at the beginning of the function. That can be enough of a clue for the optimizer.

12

As far as I can think, unsafe has two main uses : calling non-Rust functions and optimisation ( to "go faster" ). I agree wanting to "go faster" is a last resort, and is usually unnecessary, but it all depends on the context. If you want to (say) win some competition that depends very much on computing faster, it could be worth it, and not using unsafe will result in failure.

13

I think for readability in your code base it’s best to require an unsafe block around all perf_assert! invocations:

/// In debug mode or feature unsafe_opt not enabled, same as debug_assert! otherwise unsafe compiler hint.
#[cfg(any(debug_assertions, not(feature = "unsafe_opt")))]
macro_rules! perf_assert {
    ( $cond: expr ) => {
        if false {
            std::hint::unreachable_unchecked()
        } else {
            debug_assert!($cond)
        }
    };
}

/// In debug mode or feature unsafe_opt not enabled, same as debug_assert! otherwise unsafe compiler hint.
#[cfg(all(not(debug_assertions), feature = "unsafe_opt"))]
macro_rules! perf_assert {
    ( $cond: expr ) => {
        if !$cond {
            std::hint::unreachable_unchecked()
        }
    };
}

Unfortunately because I don’t think unsafe is hygienic, this does mean that $cond is unconditionally treated as beïng in an unsafe block. But still, it’s important to mark things that are unsafe.

14

Also, I’m not too keen on the name unsafe_opt, it implies that the feature itself is unsafe, which it isn’t. Perhaps call it something like experimental_opt?

15

The problem with that is I want to use forbid(unsafe_code) at the module level when the feature is not enabled ( which gives clients of the crate an assurance ). I suppose the macro could have a name that suggests danger!

16

"opt" in the "unsafe_opt" feature is meant to stand for "optimisation", not "option", but it could be read the wrong way. There probably is a better name for it. But I think it should contain unsafe, to make it clear what is involved. The crate documentation reads:

unsafe_opt : Enable unsafe optimisations in release mode.
17

Well, using it is potentially (memory) unsafe, if the module code is wrong. But if the module code is correct, it is perfectly safe. Umm... how to communicate that?

[ In the case of GHeap, it is pretty easy to see the indexes are all either checked immediately they are computed, or they are copied from a location that contains a valid index. Partly because the underlying Vec never reduces in size. But I guess that proof is too hard for a compiler to figure out. And even so, mistakes are so easy to make! ]

18

I would argue the feature is not unsafe, because unsafe, in Rust terminology, means that the code explicitly does not make the guarantee that no inputs result in UB. Your feature is not unsafe, because you do try to make this guarantee, so it’s incorrect to call it “unsafe”.

“It’s safe unless I have bugs in my code” is just normal Rust code. What you want to indicate is that it is more likely to contain more bugs that are more severe, which is synonymous with “experimental”, hence why I suggested experimental_opt (after all, if these optimizations were not experimental in some sense you would just enable them globally).

I don’t think there is that much significance to your library only using safe code. Because if one of your dependencies has a bug, your library can still segfault and behave incorrectly — even if you didn’t write unsafe yourself, you still can choose your dependencies and in a sense their unsafe is also your unsafe. Hence, the relatively minor unsafety of this particular feature need not be emphasized — it’s not like you’re eliminating unsafety altogether or anything.

19

I see what you mean. You could maybe invoke the macro like perf_assert!(unsafe, cond); or something, just to make it explicit that it can be an unsafe operation.

20

I don't agree with that. Most people are not crazy enough to use unsafe to go faster (e.g. suppressing index checking), most of the time. I know about an incident involving a web framework where there was extensive use of unsafe, and it turned out to be a bit controversial. Even though I have written the code, I don't use my own "unsafe_opt" in production (because the performance is ample anyway, so why risk it? ).