perf: Rework read deduplication with pooled read recorders

[xmakro]

View all comments

Every dependency read goes through DepGraph::read_index, which must decide whether the current task has already recorded the read. Small tasks used a linear scan; larger ones built an FxHashSet, rebuilt for every task and rehashed as it grew, while the reads themselves were collected in an EdgesVec allocated and freed per task.

Larger tasks now reuse a recorder from a global pool, so the hash set and read list allocations are amortized across tasks. EdgesVec is removed.

[Kobzol]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: f859e1d (f859e1d807a97c70b3d3a1fef0940055f766ca5a)
Base parent: 4eb6253 (4eb62535fc12dd1a63fd43a4173e224e79313c4d)

[rust-timer]

Finished benchmarking commit (f859e1d): 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
@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.3%	[0.1%, 0.5%]	10
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	2
Improvements ✅ (primary)	-2.9%	[-15.1%, -0.2%]	109
Improvements ✅ (secondary)	-2.1%	[-7.5%, -0.1%]	103
All ❌✅ (primary)	-2.6%	[-15.1%, 0.5%]	119

Max RSS (memory usage)

Results (primary 3.0%, secondary 2.7%)

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

	mean	range	count
Regressions ❌ (primary)	3.0%	[0.8%, 15.0%]	83
Regressions ❌ (secondary)	3.4%	[1.9%, 8.4%]	25
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-3.2%	[-5.0%, -2.1%]	3
All ❌✅ (primary)	3.0%	[-1.1%, 15.0%]	84

Cycles

Results (primary -4.2%, secondary -3.6%)

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.6%, 3.3%]	3
Improvements ✅ (primary)	-4.2%	[-11.2%, -1.7%]	34
Improvements ✅ (secondary)	-4.5%	[-9.2%, -1.5%]	22
All ❌✅ (primary)	-4.2%	[-11.2%, -1.7%]	34

Binary size

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

Bootstrap: 489.956s -> 490.746s (0.16%)
Artifact size: 393.31 MiB -> 394.37 MiB (0.27%)

[xmakro]

The perf results are up to date, the last push only changed comments.

One question is whether the memory usage of the amortized u32 IndexVecs is acceptable. If not, there are two fallbacks: keeping the old hashset combined with the pool should get ~40% of this PR's instruction improvement, and a bitmask should get ~66% while using 32x less memory than the u32 IndexVec.

cc @cjgillot and @nnethercote who might be interested.

[cjgillot]

Thanks for the PR. The perf improvements are great!

One question is whether the memory usage of the amortized u32 IndexVecs is acceptable. If not, there are two fallbacks: keeping the old hashset combined with the pool should get ~40% of this PR's instruction improvement, and a bitmask should get ~66% while using 32x less memory than the u32 IndexVec.

I'd suggest to start with a PR that implements the pool and keeps hashsets. Then we will be able to perf the bitset and the u32 indexvec in isolation.

View changes since this review

[nnethercote]

The icount and cycles results look fantastic! But the wall-time numbers look like a net loss, and max-rss is also worse :(

(We normally look at icounts because for most changes only icounts has enough precision to give a meaningful comparison, but for changes with big perf effects like this one, wall-time is the ultimate metric.)

[xmakro]

Thanks @cjgillot and @nnethercote for taking a look so quickly, and @Kobzol for the perf run. I changed the PR to use a hash set as suggested, and also moved the max operation to the encoder, which allows some cleanup.

The higher max-rss was expected with the IndexVec, but the higher wall-time is odd, since the PR really avoids work. With the hash set, max-rss should match master, at the cost of about half of the instructions and cycles improvement. Let's see how the timings look with a new perf run.

[Kobzol]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: d13e411 (d13e4111950b2ee041d7fe3af60157c26fd5dcd1)
Base parent: 3c00c96 (3c00c96d3af4d5b5e101e56cc161a608b21366ee)

[rust-timer]

Finished benchmarking commit (d13e411): 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
@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.2%	[0.2%, 0.2%]	4
Regressions ❌ (secondary)	0.6%	[0.1%, 1.7%]	19
Improvements ✅ (primary)	-1.4%	[-8.9%, -0.2%]	86
Improvements ✅ (secondary)	-1.2%	[-2.8%, -0.2%]	62
All ❌✅ (primary)	-1.4%	[-8.9%, 0.2%]	90

Max RSS (memory usage)

Results (primary 1.4%, 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)	1.8%	[1.4%, 2.9%]	4
Regressions ❌ (secondary)	2.4%	[2.4%, 2.5%]	2
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[-0.6%, 2.9%]	5

Cycles

Results (primary -4.8%, secondary -3.2%)

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)	-4.8%	[-6.8%, -4.1%]	6
Improvements ✅ (secondary)	-3.2%	[-3.8%, -2.8%]	4
All ❌✅ (primary)	-4.8%	[-6.8%, -4.1%]	6

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

Bootstrap: 491.018s -> 491.894s (0.18%)
Artifact size: 388.45 MiB -> 388.99 MiB (0.14%)