rustc_arena overhaul

[nnethercote]

I find the rustc_arena a bit hard to read. These commits make it better.

r? @cjgillot

[nnethercote]

Best reviewed one commit at a time.

This should not affect perf, but let's double check:

@bors try @rust-timer queue

[bors]

⌛ Trying commit bb5344a with merge f5e0ad0...

[bors]

☀️ Try build successful - checks-actions
Build commit: f5e0ad0 (f5e0ad064247fb4fdf2fc194bab67032540ca607)

[cjgillot]

cc @Zoxc as you touched this code recently.

[rust-timer]

Finished benchmarking commit (f5e0ad0): comparison URL.

Overall result: no relevant changes - no 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.

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

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.3%	[1.3%, 1.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	2
All ❌✅ (primary)	0.2%	[-1.0%, 1.3%]	2

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: 631.429s -> 631.589s (0.03%)
Artifact size: 317.33 MiB -> 317.13 MiB (-0.06%)

[Zoxc]

I'm not a huge fan of this loop. If there's some logic error, it may run more than twice and I'd prefer a panic in that case. The loop variant before #108693 is a bit better as it avoids the large loop body.

[nnethercote]

I'm not a huge fan of this loop. If there's some logic error, it may run more than twice and I'd prefer a panic in that case.

Interesting that the loop was used before #108693, I didn't know about that. If that's how it was before, and I changed it back here, that suggests it's somehow a natural way to do it. I'm not worried about a logic error, because (a) the code used to be like that, and (b) this code is hot and so naturally gets high levels of testing.

The loop variant before #108693 is a bit better as it avoids the large loop body.

The "large" loop body is less than 30 lines, which isn't very large.

The current code has all these little functions, several with a single call site, including:

• grow
• grow_and_alloc_raw
• grow_and_alloc
• alloc_raw_without_grow
• alloc_raw
• alloc

The new code has just:

• grow
• alloc_raw
• alloc

I find this much easier to understand. When I read the current code I had trouble keeping straight all these different functions, I had to write them all down and take notes to understand them.

[Zoxc]

This does increase the code size by reverting parts of #108693. The removal of grow_and_alloc and the reintroduction of the allocation loop would be the changes that does that. That may have a small performance impact.

The other changes seem fine.

[nnethercote]

This does increase the code size by reverting parts of #108693.

No: code size is slightly smaller, as the artifact size results show.

[Zoxc]

You can't use that to show the code size of these functions due to various factors like inlining, symbols size, alignment, BOLT, etc.

[nnethercote]

You can't use that to show the code size of these functions due to various factors like inlining, symbols size, alignment, BOLT, etc.

Why not? What else should I use?

[Zoxc]

Inlining can be quite chaotic. For example if these functions grew larger, LLVM may decide to not inline some other function, which could reduce code size overall. I would expect BOLT to introduce code size noise too as I don't think it uses deterministic profiling.

I usually add some non-generic functions calling the functions I'm interested in so I can inspect their assembly. For #108693 that was functions calling alloc and alloc_slice.

[nnethercote]

Inlining can be quite chaotic. For example if these functions grew larger, LLVM may decide to not inline some other function, which could reduce code size overall. I would expect BOLT to introduce code size noise too as I don't think it uses deterministic profiling.

Sure, if the source code changes later on, the generated code and its performance characteristics might also change. That's always true.

And if BOLT is non-deterministic, that's going to affect every performance measurement we have. I haven't seen any evidence for this.

Basically, if you follow this argument to its logical endpoint, it's an argument that none of the performance measurements we make can be trusted. I think that is silly.

[nnethercote]

@cjgillot: @Zoxc has some objections, I have counter-arguments: that the perf measurements look fine, and that the new code is easier to read. It's also interesting that I unintentionally reverted some parts of the code to how it used to look; to me that's suggestive that the old way was more natural.

Anyway, as reviewer you get to decide :)

[cjgillot]

The results of perf measurement do not warrant an extended discussion, so I'll take another criterion.
The new code is much simpler to understand, and that's a good thing.
@bors r+

[bors]

📌 Commit bb5344a has been approved by cjgillot

It is now in the queue for this repository.

[bors]

⌛ Testing commit bb5344a with merge 6c29b45...

[bors]

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 6c29b45 to master...

[rust-timer]

Finished benchmarking commit (6c29b45): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.8%	[3.2%, 7.2%]	3
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-1.9%	[-2.5%, -1.4%]	2
All ❌✅ (primary)	-1.6%	[-1.6%, -1.6%]	1

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: 627.128s -> 627.002s (-0.02%)
Artifact size: 273.31 MiB -> 273.33 MiB (0.01%)