Prevent very long compilation runtimes in LateBoundRegionNameCollector #83406

b-naber · 2021-03-23T11:47:52Z

On recursive types such as in the example given in #83150, the current implementation of LateBoundRegionNameCollector has very long compilation runtimes. To prevent those we store the types visited in the middle::ty::Visitor implementation of LateBoundRegionNameCollector in a SsoHashSet.

rust-highfive · 2021-03-23T11:47:55Z

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

b-naber · 2021-03-23T11:49:39Z

src/test/ui/recursion/issue-83150.rs

@@ -0,0 +1,9 @@
+fn main() {


This test passes for an unexplainable reason. When I compile this file I get the expected error:

error[E0275]: overflow evaluating the requirement `Map<&mut Map<&mut Map<&mut Map<&mut Map<.....

But the test suite cannot find an error.

i expect you to need at least // build-pass here, as the overly large types should only get created during mono item collection

lcnr

I don't think that using type length limit is the best approach here.

The issue here is that the type T is &mut Map<T_prev, closure [T_prev]> so the type size scales by 2^depth instead of just depth.

Using https://doc.rust-lang.org/nightly/nightly-rustc/rustc_data_structures/sso/struct.SsoHashSet.html in visit_ty seems like a potentially better solution to me

lcnr · 2021-03-26T17:18:11Z

src/test/ui/recursion/issue-83150.rs

@@ -0,0 +1,9 @@
+fn main() {


i expect you to need at least // build-pass here, as the overly large types should only get created during mono item collection

lcnr · 2021-03-26T19:19:15Z

compiler/rustc_middle/src/ty/print/pretty.rs

                }
                r.super_visit_with(self)
            }
+
+            // In order to prohibit infinite recursion on infinitely recursed types we count the


infinitely recursed types

nit: there are no infinite types. Types are finite trees, so infinite types are impossible.
We can get very large types though, which causes the hang here.

b-naber · 2021-03-29T12:36:45Z

@lcnr Thanks for the review.

I don't understand how you would want to use SsoHashSet. Do you want to fill SsoHashSet until some given capacity is reached and then break control?

lcnr · 2021-03-29T13:16:23Z

I don't want to break control at all here. Afaict this is not an infinite hang, this is a finite, but incredibly large perf regression.

I want to use a SsoHashSet to not look multiple times into identical parts of our type

b-naber · 2021-03-29T14:42:09Z

@lcnr Thanks, that clarifies things. You're right, this isn't a hang.

I updated the PR to use the SsoHashSet. There's still a problem with the test though. When I changed this to build-pass the test fails, but it also fails when I add an expected error to the test with the reason that the test compiled successfully?!

b-naber · 2021-03-29T15:17:29Z

There's also a problem with another test, seems as if `LateBoundRegionNameCollector' has different functionality if we skip types we've seen before.

compiler/rustc_middle/src/ty/print/pretty.rs

b-naber · 2021-03-29T19:17:33Z

@lcnr Can you take another look? What do we do about the tests?

lcnr · 2021-03-29T20:48:20Z

i think the correct attribute here is // build-fail. By default we only try to check (like cargo check) ui tests afaik.

b-naber · 2021-03-29T22:38:48Z

@lcnr Yes, that's the correct attribute. Thanks for the review and the help.

lcnr · 2021-03-30T06:32:35Z

compiler/rustc_middle/src/ty/print/pretty.rs

-        let mut collector = LateBoundRegionNameCollector(&mut self.used_region_names);
+        let mut collector = LateBoundRegionNameCollector {
+            used_region_names: &mut self.used_region_names,
+            type_collector: SsoHashSet::with_capacity(1000),


Suggested change

type_collector: SsoHashSet::with_capacity(1000),

type_collector: SsoHashSet::new(),

we don't want to allocate here by default.

lcnr · 2021-03-30T06:32:54Z

src/test/ui/recursion/issue-83150.rs

@@ -0,0 +1,11 @@
+// build-fail
+  //~^ overflow evaluating


Suggested change

//~^ overflow evaluating

//~^ overflow evaluating

lcnr · 2021-03-30T06:33:18Z

please update the PR name and description, then this should be ready after a perf run.

b-naber · 2021-03-31T20:13:21Z

@lcnr This is ready for the perf run.

bors · 2021-03-31T20:57:52Z

☔ The latest upstream changes (presumably #76814) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr · 2021-04-05T08:17:37Z

@bors try @rust-timer queue

rust-timer · 2021-04-05T08:17:38Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-04-05T08:17:46Z

⌛ Trying commit 3194b26 with merge 9fb1f198bce56ca989b0203c337bedffb89b96a1...

bors · 2021-04-05T09:07:16Z

☀️ Try build successful - checks-actions
Build commit: 9fb1f198bce56ca989b0203c337bedffb89b96a1 (9fb1f198bce56ca989b0203c337bedffb89b96a1)

rust-timer · 2021-04-05T09:07:18Z

Queued 9fb1f198bce56ca989b0203c337bedffb89b96a1 with parent b1b0a15, future comparison URL.

rust-timer · 2021-04-05T13:15:11Z

Finished benchmarking try commit (9fb1f198bce56ca989b0203c337bedffb89b96a1): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

lcnr · 2021-04-05T18:18:37Z

@bors r+

bors · 2021-04-05T18:18:38Z

📌 Commit 3194b26 has been approved by lcnr

bors · 2021-04-05T18:50:46Z

⌛ Testing commit 3194b26 with merge d203fce...

bors · 2021-04-05T21:14:22Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing d203fce to master...

estebank · 2021-05-04T17:20:01Z

Should this be backported to 1.52? @rust-lang/compiler

Context: this fixes a t-hang stable-to-stable regression that has been reported multiple times caused by big type names, the latest report being #84102

rust-highfive assigned lcnr Mar 23, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2021

b-naber commented Mar 23, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

b-naber force-pushed the issue-83510 branch from 22ab031 to 6d4f294 Compare March 24, 2021 20:02

lcnr reviewed Mar 26, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

lcnr reviewed Mar 29, 2021

View reviewed changes

compiler/rustc_middle/src/ty/print/pretty.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

b-naber force-pushed the issue-83510 branch from 08a8b83 to 1d7b642 Compare March 29, 2021 22:37

lcnr reviewed Mar 30, 2021

View reviewed changes

b-naber force-pushed the issue-83510 branch from 1d7b642 to ac03d56 Compare March 31, 2021 20:08

b-naber changed the title ~~Introduce recursion limit in LateBoundRegionNameCollector~~ Prevent very long compilation runtimes in LateBoundRegionNameCollector Mar 31, 2021

prevent very long compilation runtimes in LateBoundRegionNameCollector

3194b26

b-naber force-pushed the issue-83510 branch from ac03d56 to 3194b26 Compare March 31, 2021 21:34

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2021

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 Apr 5, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 5, 2021

bors merged commit d203fce into rust-lang:master Apr 5, 2021

rustbot added this to the 1.53.0 milestone Apr 5, 2021

b-naber deleted the issue-83510 branch April 5, 2021 21:15

b-naber mentioned this pull request Apr 22, 2021

Compile won't finish for ever with dynamic. #84102

Closed

Mark-Simulacrum mentioned this pull request May 4, 2021

add overflow hang/slow test rust-lang/rustc-perf#870

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent very long compilation runtimes in LateBoundRegionNameCollector #83406

Prevent very long compilation runtimes in LateBoundRegionNameCollector #83406

b-naber commented Mar 23, 2021 •

edited

Loading

rust-highfive commented Mar 23, 2021

b-naber Mar 23, 2021

lcnr Mar 26, 2021

This comment has been minimized.

lcnr left a comment

lcnr Mar 26, 2021

lcnr Mar 26, 2021 •

edited

Loading

b-naber commented Mar 29, 2021 •

edited

Loading

lcnr commented Mar 29, 2021 •

edited

Loading

b-naber commented Mar 29, 2021 •

edited

Loading

This comment has been minimized.

b-naber commented Mar 29, 2021

b-naber commented Mar 29, 2021

This comment has been minimized.

lcnr commented Mar 29, 2021

b-naber commented Mar 29, 2021

lcnr Mar 30, 2021

lcnr Mar 30, 2021

lcnr commented Mar 30, 2021 •

edited

Loading

b-naber commented Mar 31, 2021

bors commented Mar 31, 2021

lcnr commented Apr 5, 2021

rust-timer commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

rust-timer commented Apr 5, 2021

rust-timer commented Apr 5, 2021

lcnr commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

estebank commented May 4, 2021 •

edited

Loading

	type_collector: SsoHashSet::with_capacity(1000),
	type_collector: SsoHashSet::new(),

Prevent very long compilation runtimes in LateBoundRegionNameCollector #83406

Prevent very long compilation runtimes in LateBoundRegionNameCollector #83406

Conversation

b-naber commented Mar 23, 2021 • edited Loading

rust-highfive commented Mar 23, 2021

b-naber Mar 23, 2021

Choose a reason for hiding this comment

lcnr Mar 26, 2021

Choose a reason for hiding this comment

This comment has been minimized.

lcnr left a comment

Choose a reason for hiding this comment

lcnr Mar 26, 2021

Choose a reason for hiding this comment

lcnr Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

b-naber commented Mar 29, 2021 • edited Loading

lcnr commented Mar 29, 2021 • edited Loading

b-naber commented Mar 29, 2021 • edited Loading

This comment has been minimized.

b-naber commented Mar 29, 2021

b-naber commented Mar 29, 2021

This comment has been minimized.

lcnr commented Mar 29, 2021

b-naber commented Mar 29, 2021

lcnr Mar 30, 2021

Choose a reason for hiding this comment

lcnr Mar 30, 2021

Choose a reason for hiding this comment

lcnr commented Mar 30, 2021 • edited Loading

b-naber commented Mar 31, 2021

bors commented Mar 31, 2021

lcnr commented Apr 5, 2021

rust-timer commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

rust-timer commented Apr 5, 2021

rust-timer commented Apr 5, 2021

lcnr commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

estebank commented May 4, 2021 • edited Loading

b-naber commented Mar 23, 2021 •

edited

Loading

lcnr Mar 26, 2021 •

edited

Loading

b-naber commented Mar 29, 2021 •

edited

Loading

lcnr commented Mar 29, 2021 •

edited

Loading

b-naber commented Mar 29, 2021 •

edited

Loading

lcnr commented Mar 30, 2021 •

edited

Loading

estebank commented May 4, 2021 •

edited

Loading