memory access sanity checks: abort instead of panic

[RalfJung]

Suggested by @Mark-Simulacrum, this should help reduce the performance impact of these checks.

[rust-highfive]

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

To be clear, I'm not sure :) I think my measurements were inconclusive. But, in theory, I would expect this to be better able to inline etc.

Ideally we'd run perf.r-l.o against this but to do that we'd need to build and publish a debug assertions compiler to compare against...

[RalfJung]

But, in theory, I would expect this to be better able to inline etc.

Is that enough to land this? I don't think I'll have time to do proper benchmarking any time soon... and this code is UB if we don't abort, so not having perfect panic-based stacktraces seems acceptable.

[RalfJung]

@Mark-Simulacrum

To be clear, I'm not sure :) I think my measurements were inconclusive. But, in theory, I would expect this to be better able to inline etc.

So how do you suggest we proceed? I don't have time to do proper measurements here. Should I close the PR, or should we land this because it'll surely not degrade debug-std performance?

[Mark-Simulacrum]

I can try to find some time to prepare the try commits for perf.rlo to benchmark (by pushing to this PR), hopefully today.

[shepmaster]

r? @Mark-Simulacrum

[Mark-Simulacrum]

@bors try

[bors]

⌛ Trying commit 166480e3a0a9a89bacbf30e3abf3ac45cccb1391 with merge d94127aecf2bae628907d43c376b8fe5070431db...

[Mark-Simulacrum]

And with reverts of the first two commits..

@bors try @rust-timer queue

I think perf will only pick up the first try build to complete (or maybe neither, unsure) but we can check the other one manually anyway.

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit da345f73558c5aa1768bf0b780a5ab3573e97a79 with merge ebc6a503e956df6a5e66b34fd34b251b596d3431...

[bors]

☀️ Try build successful - checks-azure
Build commit: ebc6a503e956df6a5e66b34fd34b251b596d3431 (ebc6a503e956df6a5e66b34fd34b251b596d3431)

[rust-timer]

Queued ebc6a503e956df6a5e66b34fd34b251b596d3431 with parent f315c35, future comparison URL.

[rust-timer]

Finished benchmarking try commit (ebc6a503e956df6a5e66b34fd34b251b596d3431): comparison url.

[Mark-Simulacrum]

@rust-timer build d94127aecf2bae628907d43c376b8fe5070431db

[rust-timer]

Queued d94127aecf2bae628907d43c376b8fe5070431db with parent f315c35, future comparison URL.

[rust-timer]

Finished benchmarking try commit (d94127aecf2bae628907d43c376b8fe5070431db): comparison url.

[Mark-Simulacrum]

Okay so now both of those are done, the right thing to look at is this comparison: https://perf.rust-lang.org/compare.html?start=ebc6a503e956df6a5e66b34fd34b251b596d3431&end=d94127aecf2bae628907d43c376b8fe5070431db, which compares "debug asserts w/o this PR" and "debug asserts w/ this PR" -- it looks like it's a pretty nice win, 3-10% pretty much across the board. It's also notable that debug assertions are really expensive today regardless, 10-30% regressions on many benchmarks.

We probably don't want to apply this pattern exactly across the board -- abort() is after all harder to debug -- but I wonder if we can invent some "low-cost" debug_assert, e.g., one that emulates panic=abort or so (so we still get a backtrace potentially and such, but we still avoid the unwind).

I imagine you probably don't have the bandwidth to explore doing so? Maybe we should re-run benchmarks on a PR that changes debug_assert! to literally abort() if the condition fails to get a sense of how much performance that can win back.

cc @nnethercote as well

[RalfJung]

I imagine you probably don't have the bandwidth to explore doing so?

I'm afraid I do not. I also have no idea how you did the benchmarking here.^^ But it seems like the PR is a clear win, and only some very low-level methods are changed to not have backtraces any more, so seems like a good sign to me independent of the other changes you suggested.

[Mark-Simulacrum]

Okay I'll remove the benchmarking commits on this PR and approve it, I agree it's a sufficient win just by itself.

The way to do benchmarks like this is to get a build from just debug asserts being enabled and another build (critically with same parent on master) with debug asserts plus this PR's changes, then you compare the two try commits directly in perf.rlo.

[RalfJung]

Oh and you adjusted CI config to get debug assertions in try builds, I see.

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 81c7ebd has been approved by Mark-Simulacrum

[Mark-Simulacrum]

(Should be fine to rollup, as there's no expected impact on non-debug-assert builds).