Optimize hash map operations in the query system #115747

Zoxc · 2023-09-11T06:21:16Z

This optimizes hash map operations in the query system by explicitly passing hashes and using more optimal operations. find_or_find_insert_slot in particular saves a hash table lookup over entry. It's not yet available in a safe API, but will be in rust-lang/hashbrown#466.

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check	1.6189s	1.6129s	-0.37%
🟣 hyper:check	0.2353s	0.2337s	-0.67%
🟣 regex:check	0.9344s	0.9289s	-0.59%
🟣 syn:check	1.4693s	1.4652s	-0.28%
🟣 syntex_syntax:check	5.6606s	5.6439s	-0.30%
Total	9.9185s	9.8846s	-0.34%
Summary	1.0000s	0.9956s	-0.44%

r? @cjgillot

rustbot · 2023-09-11T06:21:26Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

cjgillot · 2023-09-21T17:03:12Z

@bors try @rust-timer queue

bors · 2023-09-21T17:03:23Z

⌛ Trying commit 936d8ad with merge d898dc6...

Optimize hash map operations in the query system This optimizes hash map operations in the query system by explicitly passing hashes and using more optimal operations. `find_or_find_insert_slot` in particular saves a hash table lookup over `entry`. It's not yet available in a safe API, but will be in rust-lang/hashbrown#466. <table><tr><td rowspan="2">Benchmark</td><td colspan="1">Before</th><td colspan="2">After</th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 clap:check</td><td align="right">1.6189s</td><td align="right">1.6129s</td><td align="right"> -0.37%</td></tr><tr><td>🟣 hyper:check</td><td align="right">0.2353s</td><td align="right">0.2337s</td><td align="right"> -0.67%</td></tr><tr><td>🟣 regex:check</td><td align="right">0.9344s</td><td align="right">0.9289s</td><td align="right"> -0.59%</td></tr><tr><td>🟣 syn:check</td><td align="right">1.4693s</td><td align="right">1.4652s</td><td align="right"> -0.28%</td></tr><tr><td>🟣 syntex_syntax:check</td><td align="right">5.6606s</td><td align="right">5.6439s</td><td align="right"> -0.30%</td></tr><tr><td>Total</td><td align="right">9.9185s</td><td align="right">9.8846s</td><td align="right"> -0.34%</td></tr><tr><td>Summary</td><td align="right">1.0000s</td><td align="right">0.9956s</td><td align="right"> -0.44%</td></tr></table> r? `@cjgillot`

bors · 2023-09-21T18:15:03Z

☀️ Try build successful - checks-actions
Build commit: d898dc6 (d898dc6b90c747dee52de63fccc7f77f86171136)

rust-timer · 2023-09-21T19:38:13Z

Finished benchmarking commit (d898dc6): comparison URL.

Overall result: ✅ improvements - 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 is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.8%	[0.7%, 0.9%]	2
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.3%]	14
Improvements ✅ (secondary)	-0.5%	[-0.9%, -0.2%]	25
All ❌✅ (primary)	-0.3%	[-0.5%, -0.3%]	14

Max RSS (memory usage)

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

Cycles

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)	3.2%	[3.1%, 3.5%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 630.615s -> 639.436s (1.40%)
Artifact size: 317.63 MiB -> 317.09 MiB (-0.17%)

[TEST] Optimize hash map operations in the query system This is a variant of rust-lang#115747 without using the `hashbrown` crate to see if that change the bootstrap impact. r? `@cjgillot`

Zoxc · 2023-09-24T23:03:24Z

This could use another perf run to see if using the sysroot hashbrown crate helps with bootstrap times.

lqd · 2023-09-25T05:44:01Z

@bors try @rust-timer queue

bors · 2023-09-25T05:45:10Z

⌛ Trying commit 54d9510 with merge 2a31958...

Optimize hash map operations in the query system This optimizes hash map operations in the query system by explicitly passing hashes and using more optimal operations. `find_or_find_insert_slot` in particular saves a hash table lookup over `entry`. It's not yet available in a safe API, but will be in rust-lang/hashbrown#466. <table><tr><td rowspan="2">Benchmark</td><td colspan="1">Before</th><td colspan="2">After</th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 clap:check</td><td align="right">1.6189s</td><td align="right">1.6129s</td><td align="right"> -0.37%</td></tr><tr><td>🟣 hyper:check</td><td align="right">0.2353s</td><td align="right">0.2337s</td><td align="right"> -0.67%</td></tr><tr><td>🟣 regex:check</td><td align="right">0.9344s</td><td align="right">0.9289s</td><td align="right"> -0.59%</td></tr><tr><td>🟣 syn:check</td><td align="right">1.4693s</td><td align="right">1.4652s</td><td align="right"> -0.28%</td></tr><tr><td>🟣 syntex_syntax:check</td><td align="right">5.6606s</td><td align="right">5.6439s</td><td align="right"> -0.30%</td></tr><tr><td>Total</td><td align="right">9.9185s</td><td align="right">9.8846s</td><td align="right"> -0.34%</td></tr><tr><td>Summary</td><td align="right">1.0000s</td><td align="right">0.9956s</td><td align="right"> -0.44%</td></tr></table> r? `@cjgillot`

cjgillot · 2023-10-07T10:34:26Z

compiler/rustc_query_system/src/query/caches.rs

@@ -19,9 +19,9 @@ pub trait QueryCache: Sized {
    type Value: Copy;

    /// Checks if the query is already computed and in the cache.
-    fn lookup(&self, key: &Self::Key) -> Option<(Self::Value, DepNodeIndex)>;
+    fn lookup(&self, key: &Self::Key, key_hash: u64) -> Option<(Self::Value, DepNodeIndex)>;


Is there a way to abstract over the (Key, u64) pair? A PreHashedKey struct to bundle both together?

Maybe. I did measure passing around the key hash separately:

Benchmark Before After
Time Time %
🟣 clap:check 1.7741s 1.7677s -0.36%
🟣 hyper:check 0.2775s 0.2759s -0.55%
🟣 regex:check 0.9941s 0.9933s -0.08%
🟣 syn:check 1.5865s 1.5878s 0.08%
🟣 syntex_syntax:check 6.1135s 6.1049s -0.14%
Total 10.7456s 10.7297s -0.15%
Summary 1.0000s 0.9979s -0.21%

It's a bit close to the noise floor, so I should rerun it with more samples. Most of the benefit of this PR seems to be the removal of the extra hashmap lookup.

bors · 2023-10-27T12:53:49Z

☔ The latest upstream changes (presumably #117272) made this pull request unmergeable. Please resolve the merge conflicts.

apiraino · 2023-11-21T16:26:58Z

hello, checking progress. @Zoxc were are comments addressed? what's the current status? I'm looking at this one and I'm not sure. Also, pending a perf. triaging. thanks!

@rustbot author

Zoxc · 2023-12-07T16:50:32Z

I've changed it to use the new hashbrown HashTable type and removed the changes to the query caches. It needs some new performance measurements.

Zoxc · 2023-12-09T20:27:19Z

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check	1.5851s	1.5787s	-0.40%
🟣 hyper:check	0.2333s	0.2329s	-0.17%
🟣 regex:check	0.8890s	0.8853s	-0.41%
🟣 syn:check	1.4557s	1.4368s	💚 -1.30%
🟣 syntex_syntax:check	5.5859s	5.5620s	-0.43%
Total	9.7489s	9.6957s	-0.55%
Summary	1.0000s	0.9946s	-0.54%

A new perf run would be useful too.

bors · 2024-01-03T21:04:13Z

☔ The latest upstream changes (presumably #119549) made this pull request unmergeable. Please resolve the merge conflicts.

Zoxc · 2024-01-18T06:55:08Z

Ping for a new perf run.

Kobzol · 2024-01-18T06:56:07Z

@bors try @rust-timer queue

Optimize hash map operations in the query system This optimizes hash map operations in the query system by explicitly passing hashes and using more optimal operations. `find_or_find_insert_slot` in particular saves a hash table lookup over `entry`. It's not yet available in a safe API, but will be in rust-lang/hashbrown#466. <table><tr><td rowspan="2">Benchmark</td><td colspan="1">Before</th><td colspan="2">After</th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 clap:check</td><td align="right">1.6189s</td><td align="right">1.6129s</td><td align="right"> -0.37%</td></tr><tr><td>🟣 hyper:check</td><td align="right">0.2353s</td><td align="right">0.2337s</td><td align="right"> -0.67%</td></tr><tr><td>🟣 regex:check</td><td align="right">0.9344s</td><td align="right">0.9289s</td><td align="right"> -0.59%</td></tr><tr><td>🟣 syn:check</td><td align="right">1.4693s</td><td align="right">1.4652s</td><td align="right"> -0.28%</td></tr><tr><td>🟣 syntex_syntax:check</td><td align="right">5.6606s</td><td align="right">5.6439s</td><td align="right"> -0.30%</td></tr><tr><td>Total</td><td align="right">9.9185s</td><td align="right">9.8846s</td><td align="right"> -0.34%</td></tr><tr><td>Summary</td><td align="right">1.0000s</td><td align="right">0.9956s</td><td align="right"> -0.44%</td></tr></table> r? `@cjgillot`

bors · 2024-01-18T06:57:17Z

⌛ Trying commit ca6f296 with merge 7e653fc...

bors · 2024-01-18T08:22:58Z

☀️ Try build successful - checks-actions
Build commit: 7e653fc (7e653fc3e20b4a90befe2f258e63896663e0e9b0)

rust-timer · 2024-01-18T09:48:19Z

Finished benchmarking commit (7e653fc): comparison URL.

Overall result: ❌✅ regressions and improvements - 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 is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

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.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	3.3%	[3.3%, 3.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-4.2%, -0.7%]	4
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	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: 663.454s -> 666.358s (0.44%)
Artifact size: 308.30 MiB -> 308.36 MiB (0.02%)

bors · 2024-04-30T15:45:57Z

☔ The latest upstream changes (presumably #124558) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2024-05-26T23:39:22Z

@Zoxc
ping from triage - can you post your status on this PR? This PR has not received an update in a few months.

bors · 2024-07-29T05:49:05Z

☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned cjgillot Sep 11, 2023