Use Box<[Word]> for word storage in DenseBitSet

[Zalathar]

View all comments

Since DenseBitSet has a fixed domain size, it doesn't need the capacity field of Vec. This shrinks the inline-size of DenseBitSet from 32 bytes to 24 bytes.

The main reason this wasn't done earlier is that GrowableBitSet does require resizing, and was using DenseBitSet as its internal representation.

Thus, most of this PR is separating the two bitset implementations, so that DenseBitSet is free to use a more appropriate storage type.

[Zalathar]

cc @panstromek

@bors try @rust-timer queue

[Zalathar]

For future work, we could potentially shrink DenseBitSet down to 16 bytes by only storing (pointer, domain_size) and inferring allocation length from the domain size. But that would require unsafe shenanigans that the implementation isn't ready for.

[rust-bors]

☀️ Try build successful (CI)
Build commit: cfa61c7 (cfa61c735d872ca7c526a0a3a312336e026fc444)
Base parent: 17fd5b8 (17fd5b8a37b6667b6cc137f3cc35f09759768a3b)

[panstromek]

Nice, I was going to try this one too, it should help cases when we store DenseBitSets in Vecs, like SparseBitMatrix. For those, it would be even better to store domain_size out of the datatructure completely and maybe store the data inline. That's part of the motivation for why I'm experimenting with making the DenseBitSet generic over storage (one experiment for this pattern is #161584) to make it possible to make these decisions per usage (and why I think your raw PR would make that easier, too).

As I was looking at the usages of these (DenseBitSets in Vecs, SparseBitMatrix), I found that they are often not quite optimally used anyway, so it might be better to just eliminate them, so I did one in #161850 and I think we can eliminate other ones too. That might reduce the impact of this PR, but we still probably have a ton of BitSets in other places where it'll help.

[rust-timer]

Finished benchmarking commit (cfa61c7): 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.5%	[-0.8%, -0.2%]	4
Improvements ✅ (secondary)	-0.2%	[-0.4%, -0.0%]	10
All ❌✅ (primary)	-0.5%	[-0.8%, -0.2%]	4

Max RSS (memory usage)

Results (primary 2.6%, 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.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	2.7%	[0.8%, 6.3%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

Cycles

Results (primary 2.3%, secondary 4.1%)

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)	9.7%	[2.1%, 14.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-4.3%, -4.3%]	2
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Binary size

Results (secondary -0.0%)

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

Bootstrap: 474.378s -> 479.269s (1.03%)
Artifact size: 402.85 MiB -> 402.83 MiB (-0.01%)

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

r? @chenyukang

rustbot has assigned @chenyukang.
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 75 candidates
• Random selection from 20 candidates

[chenyukang]

@bors r+ rollup

[rust-bors]

📌 Commit 4935ae0 has been approved by chenyukang

It is now in the queue for this repository.

[Zalathar]

This has known perf impact, so let's not roll it up.

@bors rollup=never

[rust-bors]

The bors config at rust-bors.toml is invalid in this PR. Parse error:

TOML parse error at line 87, column 5
   |
87 |     "arm64ami" = "latest-gha-runner-ami-arm64"
   |     ^
missing comma between key-value pairs, expected `,`

[Zalathar]

@Kobzol

Not sure what went wrong with the above bors error, but it seems unrelated to this PR.

@bors r=chenyukang

[rust-bors]

📌 Commit 4935ae0 has been approved by chenyukang

It is now in the queue for this repository.

[rustbot]

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]

Rebased.

@bors r=chenyukang

[rust-bors]

📌 Commit e483654 has been approved by chenyukang

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: chenyukang
Duration: 3h 10m 35s
Pushing 55c4dfe to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing eca445e (parent) -> 55c4dfe (this PR)

Test differences

Show 4 test diffs

Stage 0

• bit_set::tests::contains_loose: [missing] -> pass (J0)

Stage 1

• bit_set::tests::contains_loose: [missing] -> pass (J1)

Additionally, 2 doctest diffs were found. These are ignored, as they are noisy.

Job group index

• J0: test-x86_64-gnu-llvm-21-3, test-x86_64-gnu-llvm-22-3
• J1: test-aarch64-apple-2, test-aarch64-apple-macos-26-2, test-aarch64-gnu, test-aarch64-gnu-llvm-21-2, test-aarch64-msvc-2, test-i686-gnu-2, test-i686-gnu-nopt-2, test-i686-msvc-2, test-x86_64-gnu, test-x86_64-gnu-llvm-21-1, test-x86_64-gnu-llvm-22-1, test-x86_64-gnu-nopt, test-x86_64-gnu-stable, test-x86_64-mingw-2, test-x86_64-msvc-2

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 55c4dfed758e741620e3320ed472ff5c4140856a --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. test-x86_64-msvc-2: 1h 43m -> 2h 37m (+51.9%)
2. test-armhf-gnu: 59m 19s -> 1h 28m (+49.9%)
3. test-pr-check-1: 39m 48s -> 22m 22s (-43.8%)
4. test-x86_64-gnu-llvm-22-1: 54m 22s -> 1h 16m (+40.7%)
5. optional-test-x86_64-gnu-autodiff: 44m 33s -> 1h 1m (+37.3%)
6. test-aarch64-apple-2: 2h 10m -> 2h 58m (+37.1%)
7. test-x86_64-mingw-1: 2h 18m -> 3h 9m (+36.6%)
8. test-x86_64-gnu-gcc-core-tests: 17m 34s -> 11m 29s (-34.6%)
9. test-various: 1h 37m -> 2h 9m (+33.8%)
10. test-x86_64-msvc-1: 2h -> 2h 40m (+33.3%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (55c4dfe): comparison URL.

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

@rustbot label: -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.2%, 0.4%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.2%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.0%)

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

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.4%, 0.5%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	[-1.3%, -0.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-1.3%, 0.5%]	5

Cycles

Results (primary -0.1%, secondary 6.7%)

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

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.4%, 2.2%]	5
Regressions ❌ (secondary)	6.7%	[6.7%, 6.7%]	1
Improvements ✅ (primary)	-0.8%	[-2.2%, -0.4%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-2.2%, 2.2%]	12

Binary size

Results (primary 0.0%, secondary 0.1%)

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

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	44
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.1%]	44

Bootstrap: 481.923s -> 480.775s (-0.24%)
Artifact size: 403.28 MiB -> 403.36 MiB (0.02%)

[panstromek]

This is quite interesting, the win is much smaller now. Since this PR was first measured, we landed a few other optimizations in places where DenseBitSet is used and it looks like they addressed some of the same problems as this PR.

Still a win though, cranelift-codegen is still green, just right below the significance treshold. Some other benchmarks like serde as well.

include-blob is bimodal noise again (working on getting rid of that one! fingers crossed :D)

[Zalathar]

And regardless of the perf outcome, I'm happy to have disconnected GrowableBitSet from DenseBitSet, as the coupling was a big drag on any attempt to change DenseBitSet.