[Jamesbarford]

Create a separate function for interning each TyKind and an individual flags computation function. Doing this means that we can avoid a match statement that previously existed when computing flags which improves perf; #165396 (comment)

Separately there might be some wins with splitting out the InternedSet's (see 'Original Description' below) however, in my view, there is enough of a benefit with this relatively simple change that modifying the InternedSet's could be a separate exploration.

Edit:
My original write up for the PR is below. However what this PR has become and what the original intention was has drifted sufficiently that writing a new description made the most sense to me.

Original Description

View all comments

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%)

[Jamesbarford]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3ec83b5 (3ec83b5d60aa4b869b1b89914bcccf0e09778d50)
Base parent: 495625e (495625e3cba369dad96c4d1e8030e8fe1816ab2c)

[rust-timer]

Finished benchmarking commit (3ec83b5): 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
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.1%]	111
Improvements ✅ (secondary)	-0.6%	[-3.2%, -0.0%]	154
All ❌✅ (primary)	-0.4%	[-0.9%, -0.1%]	111

Max RSS (memory usage)

Results (primary -0.4%, secondary 2.5%)

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

	mean	range	count
Regressions ❌ (primary)	3.1%	[3.1%, 3.1%]	1
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-3.9%	[-3.9%, -3.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-3.9%, 3.1%]	2

Cycles

Results (primary -1.6%, secondary -2.4%)

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)	-	-	0
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	-1.6%	[-1.6%, -1.6%]	1

Binary size

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

Bootstrap: 482.583s -> 483.241s (0.14%)
Artifact size: 406.20 MiB -> 406.31 MiB (0.03%)

[Jamesbarford]

I've simplified the idea as I've entwined two separate ideas

1. Separate InternedSet's per TyKind
2. Separate flag computation functions and intern function calls per TyKind.

The latest commit, 1eaf80d, uses the original set up for the InternedSet's, using type_, but keeps the idea of having separate function calls for interning a TyKind along with a direct call to compute the flags. This seems to have maintained most of the performance benefits with few degradations. I think it is less convoluted than my original idea. Though it might be interesting to explore the multiple InteredSet's separately.

With this in mind.

@rustbot ready

[rust-bors]

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