[Jamesbarford]

Create an InternedSet per TyKind variant; this is something I discussed with @lqd & @lcnr separately, both thought it was a reasonable idea.

I believe the speedup comes from no longer having to check the TyKind on every call to intern_ty(...) when computing flags. This saves several function calls and branches. As such I also had to create separate flags() functions to get the flags for a TyKind so we don't have to do a match on the kind.

LLM usage;

TLDR; I prototyped a PR as I knew a lot of the implementation would be boilerplate, ran perf to see if it was worth following up on, @lqd & @lcnr thought it looked promising. Hence I've now opened this PR which is my work borrowing parts from the prototype.

Longer version
The idea is mine and I discussed it with @lcnr before doing any work, we also decided using an LLM to explore if the idea was worth pursing seemed reasonable. I used an LLM to completely vibe code the first commit of a prototype-y PR to verify the idea, which I then reviewed, and subsequently ran perf, things looked decent.

The other commits in that prototype PR were me, not an LLM, trying to effectively create the below idea;

fn compute_flags<const K: ty::TyKind<'tcx>() {
    const match K.kind() {
       // 
    }
}

Which I subsequently abandoned as I couldn't get it to work even when turning on a bunch of experimental features. I appreciate that might be me doing things wrong. I tried this as to me this idea seems to scream for compile-time evaluation as we can know the TyKind at compile time. This PR has been written by me borrowing bits from my original LLM/James collaborative prototype.

r? @nnethercote

[Jamesbarford]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: d3789d8 (d3789d895a1ea496635a79e6165e44fba0c0ecba)
Base parent: 6a98084 (6a98084e4a21b04504c034dfc640e35e32ab9854)

[rust-timer]

Finished benchmarking commit (d3789d8): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.1%, 0.5%]	8
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.2%]	90
Improvements ✅ (secondary)	-0.8%	[-3.1%, -0.2%]	83
All ❌✅ (primary)	-0.4%	[-0.9%, -0.2%]	90

Max RSS (memory usage)

Results (primary 3.0%, secondary 1.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.0%	[3.0%, 3.0%]	1
Regressions ❌ (secondary)	5.6%	[3.1%, 8.3%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.3%	[-10.0%, -1.6%]	5
All ❌✅ (primary)	3.0%	[3.0%, 3.0%]	1

Cycles

Results (primary -2.7%, secondary -1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.5%	[3.7%, 9.1%]	4
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-3.7%	[-6.9%, -2.1%]	10
All ❌✅ (primary)	-2.7%	[-2.7%, -2.7%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 482.475s -> 481.535s (-0.19%)
Artifact size: 406.38 MiB -> 406.48 MiB (0.03%)

[nnethercote]

My understanding of the LLM policy is that you are supposed to organize a reviewer in advance who is willing to review LLM-generated code, not dump it on a reviewer after talking with someone else. Nevertheless I will review this.

You mentioned boilerplate, which is a problem. That's one of the things about LLM-generated code -- you can end up with code you'd be unlikely to write as a human because it's so repetitive. Which means it's also not very nice to read.

There is also a bigger problem: some of the TyKinds only have a single value: Bool, Char, Str, Never, Error. It's silly to have a set for these variants, which will only have zero or one elements. Furthermore, some of the others have a fixed number of possible values: Int, Uint, Float. That's why values of all of these TyKinds are already pre-interned in CommonTypes.

Another problem is that some kinds are very common (e.g. Adt) while others are very rare (e.g. CoroutineWitness). I wonder if it's possible to focus on a subset of the variants to get the lion's share of the improvement with much less boilerplate.

It feels like there is a germ of an interesting idea here, but the current expression of it needs work.

View changes since this review

[Jamesbarford]

I've made a bit of a shambles of explaining myself 🤦. My initial commit in this PR was not LLM generated. But does bare similarities to a draft which I did use an LLM for, so if something here looks terrible, that's down to my own skill issues 🙃.

Thank you for the review. I think it's the declarative nature of the code with a lack of runtime branching along with the separate InternedSet's are contributing factors to the performance gains.

I've made a narrow field on the TyKindInterners with the idea of it being for something that can only have a small range of values or values are not used terribly often, as you suggested. Though I'm not sure if I have got the split right yet?

I'm pretty sure the method fn intern(...) should inline, from trying something similar out of tree it appeared to behave as I was expecting and I think is more readable than a macro.

[Jamesbarford]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 613b3d3 (613b3d37e9cc79f01d4325f1dbdd0e76214e8b98)
Base parent: 2d20b42 (2d20b42b883dcfc014f3c2c56081eeb6091c2179)

[rust-timer]

Finished benchmarking commit (613b3d3): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.1%, 0.5%]	13
Improvements ✅ (primary)	-0.5%	[-0.9%, -0.2%]	86
Improvements ✅ (secondary)	-0.8%	[-3.1%, -0.2%]	79
All ❌✅ (primary)	-0.5%	[-0.9%, -0.2%]	86

Max RSS (memory usage)

Results (primary 0.2%, secondary 1.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.0%	[2.0%, 2.0%]	2
Regressions ❌ (secondary)	5.5%	[1.5%, 9.4%]	10
Improvements ✅ (primary)	-1.5%	[-1.8%, -1.3%]	2
Improvements ✅ (secondary)	-5.2%	[-9.5%, -2.0%]	6
All ❌✅ (primary)	0.2%	[-1.8%, 2.0%]	4

Cycles

Results (primary 2.9%, secondary -0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.9%, 2.9%]	1
Regressions ❌ (secondary)	5.8%	[2.2%, 10.1%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-6.9%, -2.3%]	10
All ❌✅ (primary)	2.9%	[2.9%, 2.9%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 480.462s -> 478.824s (-0.34%)
Artifact size: 406.22 MiB -> 406.40 MiB (0.04%)