Issue 72408 nested closures exponential #72412
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
petrochenkov
added
S-waiting-on-perf
|
⌛ Trying commit b4285d523ea1fed9690327511b92de1af2505dab with merge 8b8aaee9a06074230619a80778720382df633985... |
|
☀️ Try build successful - checks-azure |
|
Queued 8b8aaee9a06074230619a80778720382df633985 with parent 82911b3, future comparison URL. |
|
Finished benchmarking try commit 8b8aaee9a06074230619a80778720382df633985, comparison URL. |
|
Pretty noticeable regression. |
petrochenkov
added
S-waiting-on-author
|
Oh well, simple hashset didn't make it. I'll see what else I can do. |
|
You could try using a BTreeSet which has much lower constant factor and reasonable (log n) asymptomatic complexity. |
|
Thanks for the hint, I'll check it out. |
|
Some set benchmarks:
Here Edit: These graphs have current set size along x axis and cost of |
VFLashM
force-pushed
the
issue-72408-nested-closures-exponential
branch
from
b4285d5
to
59eaf07
Compare
|
Let's have another perf run, shall we? |
|
As for the benchmarks these graphs have current set size along x axis and cost of |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I suspect that any set will cause noticeable regressions if it's populated on every type walk, because many of the type walks are mostly empty otherwise. |
|
Awaiting bors try build completion |
|
⌛ Trying commit 59eaf07 with merge 2cd0cee6ae0fd1184272092ef9b0fb318b249060... |
petrochenkov
added
S-waiting-on-perf
|
☀️ Try build successful - checks-azure |
|
Hi! This PR showed up in the weekly perf triage report. As expected, it resulted in a very large improvement in instruction counts on the newly added The slowdown is unfortunate, but it seems you did everything possible to mitigate it. Thanks! |
Move MiniSet to data_structures remove the need for T to be copy from MiniSet as was done for MiniMap MiniMap and MiniSet was added by rust-lang#72412 think that this can be used in rust-lang#68828
Move MiniSet to data_structures remove the need for T to be copy from MiniSet as was done for MiniMap MiniMap and MiniSet was added by rust-lang#72412 think that this can be used in rust-lang#68828
rustbot
removed
the
beta-nominated
|
Is beta reverting the change that caused this problem in the first place? I would be very unhappy about this bug being left in both 1.46 and 1.47, since it has broken multiple projects for me. |
|
@spastorino @pnkfelix @wesleywiser It's not true that the performance was always bad. There is code that worked fine up until the last stable release that now takes an unacceptably long time to compile. It was caused by another beta backport that happened just prior to a stable release, #75443. |
|
The reason for the confusion is that these issues pre-existed the breakage. It is true that there was code with bad build performance that also required "opting in" by increasing the type length limit; this change fixed those preexisting issues as well. Another fix for similar regressions is in #76928. It doesn't look like we've fixed all of them yet, see #75992. |
Mark-Simulacrum
added
the
beta-nominated
|
Taking a look at this PR, it looks like the fairly large diff is mostly due to: introduction of a small-size optimized Map structure and threading through references to it into various pieces of code. It seems like it should be fairly harmless to backport and we've not seen serious regressions in nightly (it landed 2 weeks ago) as a result, to my knowledge. I am inclined to backport this to beta, personally. I am also inclined to backport #76928 which also looks like it mostly just adds some caching. @rust-lang/compiler -- given that this is indeed fixing regressions, even if the performance problem is pre-existing, is anyone opposed to a beta backport of this PR? I will also be nominating #76928 shortly. |
…k-Simulacrum [beta] backports This backports a number of PRs to beta, not all of which have been approved (yet). * Switch to environment files to change the environment on GHA rust-lang#77418 * cache types during normalization rust-lang#76928 * Fixing memory exhaustion when formatting short code suggestion rust-lang#76598 * Issue 72408 nested closures exponential rust-lang#72412 r? `@Mark-Simulacrum`
Mark-Simulacrum
added
beta-accepted
| @@ -1,4 +1,4 @@ | |||
| error: reached the recursion limit while instantiating `test::<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Cons<Nil>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>` | |||
| error: reached the recursion limit while instantiating `test::<Cons<Cons<Cons<Cons<Cons<...>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>` | |||
There was a problem hiding this comment.
Apologies for the random drive-by comment, but aren't there too many >s? I was expecting this:
test::<Cons<Cons<Cons<Cons<Cons<...>>>>>>
There was a problem hiding this comment.
Final truncation is based on number of characters (32 each side).
There was a problem hiding this comment.
Maybe it should be smarter, though?
There was a problem hiding this comment.
It's tricky, because the part of the code that understands type's structure doesn't track its printed length. The part that truncates the text length doesn't understand the type.
…=lcnr Re-implement a type-size based limit r? lcnr This PR reintroduces the type length limit added in rust-lang#37789, which was accidentally made practically useless by the caching changes to `Ty::walk` in rust-lang#72412, which caused the `walk` function to no longer walk over identical elements. Hitting this length limit is not fatal unless we are in codegen -- so it shouldn't affect passes like the mir inliner which creates potentially very large types (which we observed, for example, when the new trait solver compiles `itertools` in `--release` mode). This also increases the type length limit from `1048576 == 2 ** 20` to `2 ** 24`, which covers all of the code that can be reached with craterbot-check. Individual crates can increase the length limit further if desired. Perf regression is mild and I think we should accept it -- reinstating this limit is important for the new trait solver and to make sure we don't accidentally hit more type-size related regressions in the future. Fixes rust-lang#125460
…=lcnr Re-implement a type-size based limit r? lcnr This PR reintroduces the type length limit added in rust-lang#37789, which was accidentally made practically useless by the caching changes to `Ty::walk` in rust-lang#72412, which caused the `walk` function to no longer walk over identical elements. Hitting this length limit is not fatal unless we are in codegen -- so it shouldn't affect passes like the mir inliner which creates potentially very large types (which we observed, for example, when the new trait solver compiles `itertools` in `--release` mode). This also increases the type length limit from `1048576 == 2 ** 20` to `2 ** 24`, which covers all of the code that can be reached with craterbot-check. Individual crates can increase the length limit further if desired. Perf regression is mild and I think we should accept it -- reinstating this limit is important for the new trait solver and to make sure we don't accidentally hit more type-size related regressions in the future. Fixes rust-lang#125460
This fixes #72408.
Nested closures were resulting in exponential compilation time.
This PR is enhancing asymptotic complexity, but also increasing the constant, so I would love to see perf run results.