-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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) |
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 r- Merge conflict in |
6bd031b
to
b187c42
Compare
Rebased. |
@bors r=varkor |
📌 Commit b187c42 has been approved by |
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
@@ -1300,7 +1300,7 @@ impl<'a, 'tcx> SizeSkeleton<'tcx> { | |||
let tail = tcx.struct_tail(pointee); | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking flags on Ty
is O(1)
, why were these changed?
@@ -708,7 +708,7 @@ impl<'a, 'gcx, 'tcx> ExistentialTraitRef<'tcx> { | |||
pub fn with_self_ty(&self, tcx: TyCtxt<'a, 'gcx, 'tcx>, self_ty: Ty<'tcx>) | |||
-> 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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@@ -1247,7 +1247,7 @@ impl<'a, 'tcx, 'gcx> ExistentialProjection<'tcx> { | |||
-> ty::ProjectionPredicate<'tcx> | |||
{ | |||
// otherwise the escaping regions would be captured by the binders | |||
assert!(!self_ty.has_escaping_regions()); | |||
debug_assert!(!self_ty.has_escaping_regions()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
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. |
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.