Consider changing assert! to debug_assert! when it calls visit_with - #53025
Conversation
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Although the perf improvements are nice, I'd want to be sure that this assertions have not been hit in practice (or would definitely be caught by another, informative assertion), because assertions can often be very useful. I'm not quite sure we'd establish this, other than reasoning about the code. Perhaps it'd be possible instead to pull these assertions further out of the loops they're in (possibly keeping the |
|
While the general idea is nice, I'm not sure about the possibility of moving these checks outside of loops, because they aren't just repeated - as far as I can tell these calls are recursive, but the objects they are called on change along the way. |
|
Okay, as these conditions are non-trivial with regards to performance and it seems likely to me that if they fail, we're going to hit other assertions (later on, but assuming we can reproduce the issue, that should be sufficient), I think it's probably fine to change these. Thanks for looking into it! @bors r+ rollup |
|
📌 Commit 6bd031bc8d7a3178a44060094d74cd67aeeb491c has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- Merge conflict in |
bors
added
S-waiting-on-author
6bd031b to
b187c42
Compare
|
Rebased. |
kennytm
added
S-waiting-on-review
|
@bors r=varkor |
|
📌 Commit b187c42 has been approved by |
bors
removed
the
S-waiting-on-review
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.
Rollup of 15 pull requests Successful merges: - #52955 (Update compiler test documentation) - #53019 (Don't collect() when size_hint is useless) - #53025 (Consider changing assert! to debug_assert! when it calls visit_with) - #53059 (Remove explicit returns where unnecessary) - #53165 ( Add aarch64-unknown-netbsd target) - #53210 (Deny future duplication of rustc-ap-syntax) - #53223 (A few cleanups for rustc_data_structures) - #53230 ([nll] enable feature(nll) on various crates for bootstrap: part 4) - #53231 (Add let keyword doc) - #53240 (Add individual documentation for <integer>`.swap_bytes`/.`reverse_bits`) - #53253 (Remove unwanted console log) - #53264 (Show that Command can be reused and remodified) - #53267 (Fix styles) - #53273 (Add links to std::char::REPLACEMENT_CHARACTER from docs.) - #53283 (wherein we suggest float for integer literals where a float was expected) Failed merges: r? @ghost
| match tail.sty { | ||
| ty::TyParam(_) | ty::TyProjection(_) => { | ||
| assert!(tail.has_param_types() || tail.has_self_ty()); | ||
| debug_assert!(tail.has_param_types() || tail.has_self_ty()); |
There was a problem hiding this comment.
Checking flags on Ty is O(1), why were these changed?
| -> ty::TraitRef<'tcx> { | ||
| // otherwise the escaping regions would be captured by the binder | ||
| assert!(!self_ty.has_escaping_regions()); | ||
| debug_assert!(!self_ty.has_escaping_regions()); |
| { | ||
| // otherwise the escaping regions would be captured by the binders | ||
| assert!(!self_ty.has_escaping_regions()); | ||
| debug_assert!(!self_ty.has_escaping_regions()); |
|
This PR seems like it's working around Unlike what @varkor said, I doubt many of these are correct, if any, due to how the situations they're checking for (e.g. "no inference variables") can have side-effects without causing an ICE. |
6 participants
The perf run from #52956 revealed that there were 3 benchmarks that benefited most from changing
assert!s todebug_assert!s: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!todebug_assert!in those places in order to get the performance wins shown by the benchmarks.