Consider changing assert! to debug_assert! when it calls visit_with #53025

ljedrz · 2018-08-03T11:33:29Z

The perf run from #52956 revealed that there were 3 benchmarks that benefited most from changing assert!s to debug_assert!s:

issue Nightly rust hangs forever #46449: avg -4.7% for -check
deeply-nested (AKA Nonlinear compile time blow-up with deeply nested types #38528): avg -3.4% for -check
regression Massive compiler perf regression #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.

rust-highfive · 2018-08-03T11:33:32Z

r? @varkor

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

varkor · 2018-08-04T00:58:24Z

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 debug_assert!s where they are, but adding additional asserts that would trigger if something went wrong, potentially at a slightly less informative location).

ljedrz · 2018-08-05T06:21:43Z

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.

varkor · 2018-08-07T11:33:08Z

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

bors · 2018-08-07T11:33:09Z

📌 Commit 6bd031bc8d7a3178a44060094d74cd67aeeb491c has been approved by varkor

kennytm · 2018-08-10T05:58:04Z

@bors r-

Merge conflict in src/librustc/traits/select.rs.

ljedrz · 2018-08-10T06:44:06Z

Rebased.

kennytm · 2018-08-10T07:38:14Z

@bors r=varkor

bors · 2018-08-10T07:38:14Z

📌 Commit b187c42 has been approved by varkor

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.

@ghost

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

eddyb · 2019-12-09T23:23:32Z

src/librustc/ty/layout.rs

@@ -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());


Checking flags on Ty is O(1), why were these changed?

eddyb · 2019-12-09T23:23:48Z

src/librustc/ty/sty.rs

@@ -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());


eddyb · 2019-12-09T23:23:53Z

src/librustc/ty/sty.rs

@@ -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());


eddyb · 2019-12-09T23:27:14Z

This PR seems like it's working around TraitRef not having its own precomputed flags?
Maybe we should be interning TraitRefs, similar to Ty?

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.
E.g. inference variables can get resolved to something that the code assumed to not be there because it 1. looked for the thing explicitly 2. it asserted there are no inference variables.

rust-highfive assigned varkor Aug 3, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2018

varkor mentioned this pull request Aug 4, 2018

Use debug_assert! instead of assert! where possible #52956

Closed

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 7, 2018

This was referenced Aug 8, 2018

Rollup of 14 pull requests #53184

Closed

Rollup of 14 pull requests #53194

Closed

Rollup of 14 pull requests #53195

Closed

Rollup of 13 pull requests #53197

Closed

cramertj mentioned this pull request Aug 8, 2018

Rollup of 12 pull requests #53206

Closed

kennytm mentioned this pull request Aug 9, 2018

Rollup of 15 pull requests #53216

Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 10, 2018

Consider changing assert! to debug_assert! when it calls visit_with

b187c42

ljedrz force-pushed the debug_asserts_limited branch from 6bd031b to b187c42 Compare August 10, 2018 06:43

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 10, 2018

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2018

kennytm mentioned this pull request Aug 10, 2018

Rollup of 23 pull requests #53252

Closed

GuillaumeGomez mentioned this pull request Aug 12, 2018

Rollup of 15 pull requests #53297

Merged

bors merged commit b187c42 into rust-lang:master Aug 13, 2018

ljedrz deleted the debug_asserts_limited branch August 13, 2018 05:39

This was referenced Aug 13, 2018

Consider using unreachable_unchecked in hot spots #52945

Closed

[experiment] use unreachable_unchecked() everywhere #53307

Closed

pnkfelix mentioned this pull request Oct 8, 2018

[1.30 beta] Test suite of the jemalloc-ctl crate is failing #54478

Closed

eddyb reviewed Dec 9, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider changing assert! to debug_assert! when it calls visit_with #53025

Consider changing assert! to debug_assert! when it calls visit_with #53025

ljedrz commented Aug 3, 2018 •

edited

Loading

rust-highfive commented Aug 3, 2018

varkor commented Aug 4, 2018 •

edited

Loading

ljedrz commented Aug 5, 2018

varkor commented Aug 7, 2018

bors commented Aug 7, 2018

kennytm commented Aug 10, 2018

ljedrz commented Aug 10, 2018

kennytm commented Aug 10, 2018

bors commented Aug 10, 2018

eddyb Dec 9, 2019

eddyb Dec 9, 2019

eddyb Dec 9, 2019

eddyb commented Dec 9, 2019

Consider changing assert! to debug_assert! when it calls visit_with #53025

Consider changing assert! to debug_assert! when it calls visit_with #53025

Conversation

ljedrz commented Aug 3, 2018 • edited Loading

rust-highfive commented Aug 3, 2018

varkor commented Aug 4, 2018 • edited Loading

ljedrz commented Aug 5, 2018

varkor commented Aug 7, 2018

bors commented Aug 7, 2018

kennytm commented Aug 10, 2018

ljedrz commented Aug 10, 2018

kennytm commented Aug 10, 2018

bors commented Aug 10, 2018

eddyb Dec 9, 2019

Choose a reason for hiding this comment

eddyb Dec 9, 2019

Choose a reason for hiding this comment

eddyb Dec 9, 2019

Choose a reason for hiding this comment

eddyb commented Dec 9, 2019

ljedrz commented Aug 3, 2018 •

edited

Loading

varkor commented Aug 4, 2018 •

edited

Loading