[Question] Is Array Indexing Unsafe in Release Mode?

[zicklag]

Hey there! I was looking into this library and I found this line in the docs:

Indexing assumes that the coordinates are in-bounds of the array, panicking otherwise. Bounds checking is only enabled in debug mode.

I was wondering what that meant would happen when indexing out-of-bounds on release builds. Would indexing out-of-bounds be unsafe in release builds?

[bonsairobo]

Here is the code in question.

In release builds indexing out of bounds is undefined behavior. This is here to avoid overhead of bounds checking that happens in slice::get.

[zicklag]

It seems like maybe that should be marked unsafe, then. I would be surprised to be able to cause undefined behavior with 100% safe rust, regardless of what optimization level I compile it at. ( Rust bugs excluded )

Maybe bounds checking could be enabled by default, but there could be an unsafe variant of the function, similar to the Rust slice API?

I personally would rather have it be safe and take the performance hit most of the time, unless I have a situaion where I really need it and benchmarks prove that it is indeed valuable. And at the very least I would want it marked as unsafe if it could cause unefined behavior.

[bonsairobo]

Fair points. I'll see if there is any appreciable regression in benchmarks from turning on bounds checking. If not, I'll just make it safe.

There actually used to be a unsafe versions of the Get traits, but it was cumbersome to maintain the extra GetUnchecked, GetRefUnchecked and GetMutUnchecked traits.

[bonsairobo]

Well unfortunately it seems that using "intrinsic slice indexing" instead of get_unchecked gives about a 50% regression in benchmarks that rely heavily on indexing.

[zicklag]

Oh, bummer. Well, at least there's nothing stopping us from using the get_unchecked for the algorithms implemented inside of building_blocks. That way users get the performance for those algorithms, and no unsafe for users, unless they need to implement thier own algorithms, at which point they can measure the cost for themselves.

On another note, I've seen instances on the Rust users forum where people have said that Rust can, in many circumstances, compile out the bounds checks completely, but sometimes it requires organizing the code differently so that rustc can make the determination that the index will never go out of bounds. I'm not sure if that helps us with the algorithms you are working with or not, but here's a relevant thread: https://users.rust-lang.org/t/performance-of-array-access-vs-c/43522/13?u=zicklag

[bonsairobo]

I'll try bringing back the unsafe traits, but I will only implement them where absolutely necessary, like for Channel and Array.

ChunkMap shouldn't need it because if you access out of bounds, you will always get an ambient value, so that's known to be safe to use get_unchecked internally without exposing an unsafe getter.

[zicklag]

Cool, sounds like a good idea. :+1:

[bonsairobo]

I think I've addressed this issue as much as I can in 6f7b86775.

Now there are both checked and unchecked variants of the Get* traits. Unchecked variants are marked unsafe. Most algorithms in this library are able to verify they're not accessing out of bounds with a single check outside of any tight loops.

[zicklag]

Yay! :tada: Thanks!