Re-implement a type-size based limit

[compiler-errors]

r? lcnr

This PR reintroduces the type length limit added in #37789, which was accidentally made practically useless by the caching changes to Ty::walk in #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 #125460

[compiler-errors]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 6ae5d95 with merge 5f700d6...

[compiler-errors]

@bors try

[bors]

⌛ Trying commit 09b9c08 with merge 61a9ac6...

[bors]

☀️ Try build successful - checks-actions
Build commit: 61a9ac6 (61a9ac64344e91d62f5496627ff363d177f9daab)

[compiler-errors]

@craterbot run mode=build-and-test

[craterbot]

👌 Experiment pr-125507 created and queued.
🤖 Automatically detected try build 61a9ac6
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[rust-timer]

Finished benchmarking commit (61a9ac6): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Warning ⚠: The following benchmark(s) failed to build:

• deeply-nested-multi

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.2%	[0.4%, 1.9%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-0.6%	[-1.0%, -0.3%]	4
All ❌✅ (primary)	1.0%	[-0.2%, 1.9%]	7

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 674.001s -> 680.65s (0.99%)
Artifact size: 315.67 MiB -> 315.56 MiB (-0.04%)

[craterbot]

🚧 Experiment pr-125507 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-125507 is completed!
📊 374 regressed and 126 fixed (465734 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[lcnr]

want to try bumping the limit x10 and rerun crater?

[kornelski]

This has broken real-world code for me (hard compile error, not just performance). Here's a minimized version:

use itertools::Itertools;

pub fn blah() {
    let _ = "".split("")
        .flat_map(|l| l.split(""))
        .flat_map(|l| l.split(""))
        .flat_map(|l| l.split(""))
        .flat_map(|l| l.split(""))
        .flat_map(|l| l.split(""))
        .flat_map(|l| l.split(""))
        .join("");
}

rust/library/core/src/iter/adapters/flatten.rs:653:9
|
653 | self.iter_try_fold(init, flatten(fold))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider adding a #![type_length_limit="30043568"] attribute to your crate

rustc 1.81.0-nightly (aa1d4f6 2024-07-03)

[lcnr]

• related to Prevent exponentially large type sizes in tuple_combinations rust-itertools/itertools#945

Itertools ends up with very large closure types, so this fels unsurprising and intended (as these large closures significantly degrade performance during canonicalization and can easily result in defacto hangs, especially with the new solver).

I hope you're able to patch the affected crate and increase its type length limit. Alternatively it may be worth it to patch itertools to outline the relevant closure here as well, similar to the PR mentioned aboe

[kornelski]

Ok, eventually it compiled, but I had to set max type length to 11GB (this repro function is part of another iterator…)

super deep stack trace

<hundreds omitted>
_RNvNtCs7ECJq7gLGVa_18rustc_monomorphize9collector17collect_items_rec
_RNvNtCs7ECJq7gLGVa_18rustc_monomorphize9collector17collect_items_rec
_RNvNtCs7ECJq7gLGVa_18rustc_monomorphize9collector17collect_items_rec
 _RNvNtCs7ECJq7gLGVa_18rustc_monomorphize9collector17collect_items_rec
  _RNvNtCs7ECJq7gLGVa_18rustc_monomorphize9collector17collect_items_rec
   _RNvNtCs7ECJq7gLGVa_18rustc_monomorphize9collector25collect_items_of_instance
    _RNvXs0_NtCs7ECJq7gLGVa_18rustc_monomorphize9collectorNtB5_16MirUsedCollectorNtNtNtCs6voQ6brMM63_12rustc_middle3mir5visit7Visitor16visit_terminator
     _RNvMs2_NtNtCs6voQ6brMM63_12rustc_middle2ty8instanceNtB5_8Instance14expect_resolve
      _RNvMs2_NtNtCs6voQ6brMM63_12rustc_middle2ty8instanceNtB5_8Instance11try_resolve
       _RINvXsp_NtNtCs6voQ6brMM63_12rustc_middle2ty16structural_implsNtB8_2TyINtNtCscfpqVFAc5KA_13rustc_type_ir5visit18TypeSuperVisitableNtNtB8_7context6TyCtxtE16super_visit_withNtNvNtB8_8instance11type_length7VisitorEBa_
        _RINvXsp_NtNtCs6voQ6brMM63_12rustc_middle2ty16structural_implsNtB8_2TyINtNtCscfpqVFAc5KA_13rustc_type_ir5visit18TypeSuperVisitableNtNtB8_7context6TyCtxtE16super_visit_withNtNvNtB8_8instance11type_length7VisitorEBa_
         _RINvXsp_NtNtCs6voQ6brMM63_12rustc_middle2ty16structural_implsNtB8_2TyINtNtCscfpqVFAc5KA_13rustc_type_ir5visit18TypeSuperVisitableNtNtB8_7context6TyCtxtE16super_visit_withNtNvNtB8_8instance11type_length7VisitorEBa_
          _RINvXsp_NtNtCs6voQ6brMM63_12rustc_middle2ty16structural_implsNtB8_2TyINtNtCscfpqVFAc5KA_13rustc_type_ir5visit18TypeSuperVisitableNtNtB8_7context6TyCtxtE16super_visit_withNtNvNtB8_8instance11type_length7VisitorEBa_
           _RINvXsp_NtNtCs6voQ6brMM63_12rustc_middle2ty16structural_implsNtB8_2TyINtNtCscfpqVFAc5KA_13rustc_type_ir5visit18TypeSuperVisitableNtNtB8_7context6TyCtxtE16super_visit_withNtNvNtB8_8instance11type_length7VisitorEBa_

[saethlin]

The attribute required to make the code in #127346 compile is quite silly.

#![type_length_limit = "30491060267835378"]

Given that value is just over 2 << 53, I think we might have a deeper problem here. The type length limit is stored in a usize which means computing that type length will overflow on a 32-bit system, and we are getting disturbingly close to overflowing a 64-bit integer in Real Code. Peak memory usage on my system from building this crate is 1.5 GB. Given that we have a type length which is many factors of 2 beyond the memory usage of the entire compiler, does anyone think that indicates some kind of flaw in the reasoning that leads to the type length calculation? I've read over the code and it looks sensible to me; the only thing I can think of is that could produce this kind of blowup is many uses of the same interned Ty, but still... my intuition says that alone can't get us to 2 << 53.

Here's the implementation:

rust/compiler/rustc_middle/src/ty/instance.rs

Lines 391 to 422 in ed7e35f

	fn type_length<'tcx>(item: impl TypeVisitable<TyCtxt<'tcx>>) -> usize {
	struct Visitor<'tcx> {
	type_length: usize,
	cache: FxHashMap<Ty<'tcx>, usize>,
	}
	impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for Visitor<'tcx> {
	fn visit_ty(&mut self, t: Ty<'tcx>) {
	if let Some(&value) = self.cache.get(&t) {
	self.type_length += value;
	return;
	}
	let prev = self.type_length;
	self.type_length += 1;
	t.super_visit_with(self);
	// We don't try to use the cache if the type is fairly small.
	if self.type_length > 16 {
	self.cache.insert(t, self.type_length - prev);
	}
	}
	fn visit_const(&mut self, ct: ty::Const<'tcx>) {
	self.type_length += 1;
	ct.super_visit_with(self);
	}
	}
	let mut visitor = Visitor { type_length: 0, cache: Default::default() };
	item.visit_with(&mut visitor);
	visitor.type_length
	}

Am I missing something obvious?

[jackh726]

This seems to be causing problems for people. We should consider reverting this as we assess the impact and correctness.

[RossSmyth]

The Helix editor no longer compiles on the latest nightly (154fdac39 2024-07-07). I believe it is because of this PR

[Kobzol]

This PR heavily regressed a stress test that contains very deeply nested types, but this perf. regression has since been fixed in #127288. Regarding the performance regression itself, I consider it to be resolved.

@rustbot label: +perf-regression-triaged

[Noratrieb]

@saethlin having 53 level nested 2-element tuples is enough to reach 2<<53. That's just 53 distinct types, which, thanks to caching, is easily processed by the rest of the compiler.

edit: this appears to not be that simple from my testing, such nested tuples do OOM the compiler :3