Add AllowInternalUnstable enum to avoid Arc clones for static desugaring feature lists

[spastorino]

View all comments

This is exploratory followup of #162747

There are upsides and downsides of this. In particular, the bad part is that we grow ExpnData size by 8 bytes. The rest I think is way better this way.
Maybe we can do a perf run and see if it make things worse and decide.

r? @nnethercote

[spastorino]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: f9f1546 (f9f1546379ba808fc075d149550371b9613a0314)
Base parent: 420ed2a (420ed2a0c3d7225b1744266fd884d431b4d8cfe0)

[rust-timer]

Finished benchmarking commit (f9f1546): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@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.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.1%]	3
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	1

Max RSS (memory usage)

Results (primary -0.0%, secondary -0.9%)

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

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.8%, 1.4%]	2
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-2.4%	[-3.6%, -1.2%]	2
All ❌✅ (primary)	-0.0%	[-2.2%, 1.4%]	3

Cycles

Results (primary 3.2%, secondary -0.5%)

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

	mean	range	count
Regressions ❌ (primary)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	3.2%	[3.2%, 3.2%]	1

Binary size

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

Bootstrap: 498.333s -> 500.939s (0.52%)
Artifact size: 408.93 MiB -> 408.97 MiB (0.01%)

[nnethercote]

TBH I don't think this is worth it? If it improved performance, then yes. But without a perf improvement it's just a bit more complexity and code for no real gain.

[spastorino]

This was originally an experiment, not very important and we can close it. My reasoning was different I feel the code is better in the sense that we were using an Arc in a very artificial way when what we really needed was a way to split between statically created lists of strs and dynamic ones. What the new enum does is just introducing this idea. I was more on the side of, if this doesn’t make perf worse is probably worth merging.

[nnethercote]

Sometimes a struct contains a String field that might be a static string or a dynamic string. If it helps performance you might change it to Cow<'static, str>, but you probably wouldn't bother otherwise, even though the Cow more clearly communicates the "sometimes static, sometimes dynamic" nature.

This case feels similar, except that it requires defining a new type instead of reaching for something that already exists in std, plus it increases the size of ExpnData.

[spastorino]

Hold on, maybe I did not understand what you meant but the payload of Cow would be [Symbol], which would work great when borrowing and would be quite similar to my Static case.
The problem is that the owned variant would be Vec and ExpnData is cloned a lot. In my case the Dynamic variant carries an Arc<[Symbol]> and cloning is not a problem.
And another thing is that Decodable for Cow<'static, [T]> always produces Cow::Owned, so every ExpnData decoded from metadata would hit the allocating path, not just the ones from #[allow_internal_unstable] attributes.
I think those are key distinctions here.

[nnethercote]

Sorry for the confusion. Cow is not appropriate for this PR.

I mentioned Cow and String as an analogy: sometimes you use String because it's simple and good enough even though Cow is a tiny bit more efficient and exact when static strings are involved.

Likewise, in this case, I think Arc<[Symbol]> is simple and good enough and the custom AllowInternalUnstable isn't worth it.

[spastorino]

I've pushed a quick and dirty change to Cow, for the sake of trying perf over it. I don't like the idea too much and I'm not sure if our test cases would cover potential perf regressions but we would be cloning Vecs now instead of Arcs which won't be cheap.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 6e868e7 (6e868e7da4742f84f45388925ae29d92c9766cb4)
Base parent: 9a9d194 (9a9d1946d1e119676b7a6a8a26a264061e61027d)

[rust-timer]

Finished benchmarking commit (6e868e7): 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.4%	[0.2%, 2.1%]	152
Regressions ❌ (secondary)	0.5%	[0.2%, 1.0%]	72
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.3%]	10
All ❌✅ (primary)	0.4%	[0.2%, 2.1%]	152

Max RSS (memory usage)

Results (primary 3.5%, secondary -1.8%)

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

	mean	range	count
Regressions ❌ (primary)	3.5%	[1.1%, 5.9%]	2
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.8%, -2.5%]	2
All ❌✅ (primary)	3.5%	[1.1%, 5.9%]	2

Cycles

Results (primary 7.7%, secondary 4.2%)

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

	mean	range	count
Regressions ❌ (primary)	7.7%	[1.4%, 11.7%]	4
Regressions ❌ (secondary)	4.2%	[2.1%, 6.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	7.7%	[1.4%, 11.7%]	4

Binary size

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

Bootstrap: 489.957s -> 490.077s (0.02%)
Artifact size: 407.04 MiB -> 407.03 MiB (-0.00%)

[spastorino]

@nnethercote I'd leave this up to you, I can squash commits and have this ready to be merged. I still prefer my original version as we clone arcs instead of vecs.

Perf is not showing bad things but I'm quite sure that we are not exercising the bad paths. We would need macro heavy and cross crate heavy to exercise a bunch of vec clone calls. As Decodable for Cow yields Cow::Owned.

[nnethercote]

I really caused confusion by mentioning Cow :(

Did you see my comment above? That's where I tried to clarify what I was talking about.

[spastorino]

I see, yeah using Cow didn't make sense to me.
While I do prefer the proposed original change is not an important thing and you are the reviewer 😊.