Use debug_assert! instead of assert! where possible #52956
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Some of these asserts are inside unsafe code and my initial (uninformed) glance is they are guarding against undefined behaviour occuring. |
|
I don't think this is the correct approach to doing something like this. We have hundreds of issues that report the compiler ICE-ing today -- that means that panics and asserts do in fact catch bugs in shipped versions of the compiler. Furthermore, it's unclear whether a thorough review of each assert was done to determine whether it is valid to turn it into a debug_assert (read: can't cause misbehavior of the compiler, including not failing to compile invalid programs). I do think that if you're interested a PR that is more limited in scope and perhaps with documentation (in commit messages, ideally) as to why a certain group of changes is reasonable would be better received (at least by me). |
|
@Mark-Simulacrum sounds reasonable; I picked a pretty broad scope to gather opinions (e.g. if it is a plain bad idea for some modules) and ideally to do a perf run of a try build to know if this makes a big performance impact, i.e. that it is worthwhile to investigate this further and where (mir? nll?). |
|
Ideally perf runs should be done locally for investigative work (perf.rlo doesn't really present fine enough granularity). We can try, though. @bors try |
Use debug_assert! instead of assert! where possible We are using a *lot* of `assert!`ions in the compiler. I propose using `debug_assert!`s where possible instead, for the following reasons: 1. `assert!`s are not free - they incur both compilation and runtime performance penalties 2. test builds should be run with debug assertions on; `debug_assert!`s should suffice to notify us when something is wrong 3. the compiler code (besides `core`/`std` etc.) is not user-facing, so we don't have to be prepared e.g. for invalid arguments coming from runtime input 4. regular `assert!`s panic during runtime, so it's not like we can allow such behavior anyway - we need to ensure it doesn't happen before we ship I didn't alter tests (no runtime penalty), user-facing code (it requires a more thorough analysis; I might do it if this proposal passes) and a few modules I am unfamiliar with.
|
☀️ Test successful - status-travis |
|
The build was successful, but I can't seem to be able to do a comparison of 6798574 against its parent in the perf page; do perf runs of try builds need to be scheduled manually? |
|
@rust-timer build 6798574 |
kennytm
added
the
S-waiting-on-perf
|
Success: Queued 6798574 with parent 11f812a, comparison URL. |
|
Worth mentioning here that #53025 has been opened with some specific cases. Personally, I would prefer some justification other than simply performance for why these changes are unlikely to cause regressions in errors, but I'd welcome other opinions. |
kennytm
removed
the
S-waiting-on-perf
|
I think this should be done on case by case basis when profiling shows there's a problem So my disposition so far is to close this PR and send PRs like #53025 for non-trivial conditions or asserts showing up on profiles. |
petrochenkov
added
S-waiting-on-author
|
@petrochenkov Agreed - trivial assertions should be perfectly fine, even in hot spots. I'm happy that the perf run was able to indicate which assertions are complex; it would be much more difficult to determine it by just browsing the code. This PR has served its purpose. |
Consider changing assert! to debug_assert! when it calls visit_with The perf run from rust-lang#52956 revealed that there were 3 benchmarks that benefited most from changing `assert!`s to `debug_assert!`s: - issue rust-lang#46449: avg -4.7% for -check - deeply-nested (AKA rust-lang#38528): avg -3.4% for -check - regression rust-lang#31157: avg -3.2% for -check I analyzed their fixing PRs and decided to look for potentially heavy assertions in the files they modified. I noticed that all of the non-trivial ones contained indirect calls to `visit_with()`. It might be a good idea to consider changing `assert!` to `debug_assert!` in those places in order to get the performance wins shown by the benchmarks.
Consider changing assert! to debug_assert! when it calls visit_with The perf run from rust-lang#52956 revealed that there were 3 benchmarks that benefited most from changing `assert!`s to `debug_assert!`s: - issue rust-lang#46449: avg -4.7% for -check - deeply-nested (AKA rust-lang#38528): avg -3.4% for -check - regression rust-lang#31157: avg -3.2% for -check I analyzed their fixing PRs and decided to look for potentially heavy assertions in the files they modified. I noticed that all of the non-trivial ones contained indirect calls to `visit_with()`. It might be a good idea to consider changing `assert!` to `debug_assert!` in those places in order to get the performance wins shown by the benchmarks.
We are using a lot of
assert!ions in the compiler. I propose usingdebug_assert!s where possible instead, for the following reasons:assert!s are not free - they incur both compilation and runtime performance penaltiesdebug_assert!s should suffice to notify us when something is wrongcore/stdetc.) is not user-facing, so we don't have to be prepared e.g. for invalid arguments coming from runtime inputassert!s panic during runtime, so it's not like we can allow such behavior anyway - we need to ensure it doesn't happen before we shipI didn't alter tests (no runtime penalty), user-facing code (it requires a more thorough analysis; I might do it if this proposal passes) and a few modules I am unfamiliar with.