Use a safe `BucketIndex` abstraction in `VecCache` by Zalathar · Pull Request #153434 · rust-lang/rust

Zalathar · 2026-03-05T07:19:58Z

The current code for indexing into bucket arrays is quite tricky and unsafe, partly because it has to keep manually assuring the compiler that a bucket index is always less than 21.

By encapsulating that knowledge in a 21-value enum, we can make the code clearer and safer, without giving up performance.

Having a dedicated BucketIndex type could also help with further cleanups of VecCache indexing.

rustbot · 2026-03-05T07:20:04Z

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 69 candidates
Random selection from 17 candidates

Zalathar · 2026-03-05T07:20:27Z

Previous perf runs at Use a BucketIndex abstraction in VecCache #153381.

@bors try @rust-timer queue

Use a safe `BucketIndex` abstraction in `VecCache`

rust-bors · 2026-03-05T09:29:52Z

☀️ Try build successful (CI)
Build commit: a141722 (a141722f73cfed3d09e312058bc234370a64db49, parent: f8704be04fe1150527fc2cf21dd44327f0fe87fb)

Zalathar · 2026-03-05T10:42:41Z

Hmm, did my harmless tweaks somehow trigger another regression?

Zalathar · 2026-03-05T13:29:17Z

@bors try @rust-timer queue

Use a safe `BucketIndex` abstraction in `VecCache`

rust-bors · 2026-03-05T15:39:24Z

☀️ Try build successful (CI)
Build commit: a6e26e0 (a6e26e04fe48bab5cea884f2973be488f752b03e, parent: 70d86e3abeecf3a655264d9a716c5d08160176b7)

rust-timer · 2026-03-05T16:19:39Z

Finished benchmarking commit (a6e26e0): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible 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 (primary 2.3%)

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

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

Cycles

Results (primary -1.3%)

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

Binary size

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

Bootstrap: 480.787s -> 481.672s (0.18%)
Artifact size: 395.02 MiB -> 394.99 MiB (-0.01%)

rustbot · 2026-03-24T04:11:28Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Zalathar · 2026-03-24T04:14:58Z

No changes in the rebase, but let's do a more up-to-date perf run as a precaution.

@bors try @rust-timer queue

Use a safe `BucketIndex` abstraction in `VecCache`

nnethercote

Good stuff: less use of unsafe, less repetition, more comments, nice abstractions, more tests. Just a few minor things to look at.

View changes since this review

nnethercote · 2026-03-24T06:02:48Z

+#[derive(Clone, Copy, PartialEq, Eq)]
+#[repr(usize)]
+enum BucketIndex {
+    // tidy-alphabetical-start


Not sure this tidy directive is doing anything useful? I don't imagine this list will be changing.

It's mostly there to reassure readers that the list is indeed numerically sorted.

nnethercote · 2026-03-24T06:04:37Z

        // OK, now under the allocator lock, if we're still null then it's definitely us that will
        // initialize this bucket.
        if ptr.is_null() {
-            let bucket_len = SlotIndex { bucket_idx, index_in_bucket: 0 }.entries();


I think bucket_len was a misleading name? I.e. this is really the capacity.

It was arguably correct in the sense that it's the length of the bucket, but yeah I still found it misleading in practice.

nnethercote · 2026-03-24T06:09:55Z

+    for i in BucketIndex::iter_all() {
+        total += u128::try_from(i.capacity()).unwrap();
+    }
+    assert_eq!(total, (u32::MAX as u128) + 1);


The + 1 seems wrong? Every bucket has an even number of entries, and the sum should be u32::MAX.

Relatedly, there is an existing comment "The total number of entries if all buckets are initialized is u32::MAX-1." The "-1" is incorrect and should be removed.

The u128 and try_from also seems like overkill here given that we know the largest bucket is 2^31.

+1 is correct, but I've replaced it with 1 << 32 instead.

(Remember that u32::MAX == (1 << 32) - 1, which is 1 less than the total we're trying to count to.)

u128 is overkill, so I've narrowed it to u64. Using usize would overflow on 32-bit hosts, though I don't know if we ever run unit tests on those.

rust-bors · 2026-03-24T06:25:33Z

☀️ Try build successful (CI)
Build commit: ac46537 (ac465371eaf358a7445ce6b514d6b4463309d826, parent: 6f22f61305478df09f9a4523743f85d9f558c3d7)

The current code for indexing into bucket arrays is quite tricky and unsafe, partly because it has to keep manually assuring the compiler that a bucket index is always less than 21. By encapsulating that knowledge in a 21-value enum, we can make the code clearer and safer, without giving up performance. Having a dedicated `BucketIndex` type could also help with further cleanups of `VecCache` indexing.

Zalathar · 2026-03-24T07:01:50Z

Changes since review (diff):

Removed the existing comment on SlotIndex::from_index, as was largely redundant with explanations elsewhere, and incorrectly mentioned u32::MAX - 1 instead of u32::MAX.
Fixed an incorrect u32::MAX-1 on VecCache::buckets.
Clarified the bucket_index_capacity test a bit.
Renamed the vec_cache_empty test to vec_cache_empty_exhaustive, to make it easier to manually skip the long-running exhaustive tests with --skip=exhaustive.

rust-timer · 2026-03-24T07:05:13Z

Finished benchmarking commit (ac46537): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible 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)

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

Cycles

Results (primary -4.2%, secondary -0.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)	2.9%	[2.9%, 2.9%]	1
Improvements ✅ (primary)	-4.2%	[-5.0%, -3.7%]	6
Improvements ✅ (secondary)	-2.1%	[-2.2%, -2.0%]	2
All ❌✅ (primary)	-4.2%	[-5.0%, -3.7%]	6

Binary size

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

Bootstrap: 485.277s -> 483.584s (-0.35%)
Artifact size: 396.91 MiB -> 394.92 MiB (-0.50%)

nnethercote · 2026-03-24T08:30:05Z

Yes, I was mixing up u32::MAX and 2^32. Rookie mistake!

@bors r+ rollup=maybe

rust-bors · 2026-03-24T08:30:09Z

📌 Commit fd3b22a has been approved by nnethercote

It is now in the queue for this repository.

…uwer Rollup of 4 pull requests Successful merges: - #153434 (Use a safe `BucketIndex` abstraction in `VecCache`) - #154133 (Defer codegen for the VaList Drop impl to actual uses) - #154297 (fix/extend some mir-opt comments) - #154299 (document some functions on AttributeExt)

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 5, 2026

rustbot assigned wesleywiser Mar 5, 2026