Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize hash map operations in the query system #115747

Closed
wants to merge 1 commit into from

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Sep 11, 2023

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.

BenchmarkBeforeAfter
TimeTime%
🟣 clap:check1.6189s1.6129s -0.37%
🟣 hyper:check0.2353s0.2337s -0.67%
🟣 regex:check0.9344s0.9289s -0.59%
🟣 syn:check1.4693s1.4652s -0.28%
🟣 syntex_syntax:check5.6606s5.6439s -0.30%
Total9.9185s9.8846s -0.34%
Summary1.0000s0.9956s -0.44%

r? @cjgillot

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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 Sep 11, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2023

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
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 21, 2023
@bors
Copy link
Contributor

bors commented Sep 21, 2023

⌛ Trying commit 936d8ad with merge d898dc6...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2023
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"><b>Before</b></th><td colspan="2"><b>After</b></th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 <b>clap</b>:check</td><td align="right">1.6189s</td><td align="right">1.6129s</td><td align="right"> -0.37%</td></tr><tr><td>🟣 <b>hyper</b>:check</td><td align="right">0.2353s</td><td align="right">0.2337s</td><td align="right"> -0.67%</td></tr><tr><td>🟣 <b>regex</b>:check</td><td align="right">0.9344s</td><td align="right">0.9289s</td><td align="right"> -0.59%</td></tr><tr><td>🟣 <b>syn</b>:check</td><td align="right">1.4693s</td><td align="right">1.4652s</td><td align="right"> -0.28%</td></tr><tr><td>🟣 <b>syntex_syntax</b>: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
Copy link
Contributor

bors commented Sep 21, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

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%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 21, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2023
[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`
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Zoxc
Copy link
Contributor Author

Zoxc commented Sep 24, 2023

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

@lqd
Copy link
Member

lqd commented Sep 25, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 25, 2023
@bors
Copy link
Contributor

bors commented Sep 25, 2023

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 25, 2023
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"><b>Before</b></th><td colspan="2"><b>After</b></th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 <b>clap</b>:check</td><td align="right">1.6189s</td><td align="right">1.6129s</td><td align="right"> -0.37%</td></tr><tr><td>🟣 <b>hyper</b>:check</td><td align="right">0.2353s</td><td align="right">0.2337s</td><td align="right"> -0.67%</td></tr><tr><td>🟣 <b>regex</b>:check</td><td align="right">0.9344s</td><td align="right">0.9289s</td><td align="right"> -0.59%</td></tr><tr><td>🟣 <b>syn</b>:check</td><td align="right">1.4693s</td><td align="right">1.4652s</td><td align="right"> -0.28%</td></tr><tr><td>🟣 <b>syntex_syntax</b>: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`
@apiraino
Copy link
Contributor

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

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Zoxc
Copy link
Contributor Author

Zoxc commented Dec 7, 2023

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
Copy link
Contributor Author

Zoxc commented Dec 9, 2023

BenchmarkBeforeAfter
TimeTime%
🟣 clap:check1.5851s1.5787s -0.40%
🟣 hyper:check0.2333s0.2329s -0.17%
🟣 regex:check0.8890s0.8853s -0.41%
🟣 syn:check1.4557s1.4368s💚 -1.30%
🟣 syntex_syntax:check5.5859s5.5620s -0.43%
Total9.7489s9.6957s -0.55%
Summary1.0000s0.9946s -0.54%

A new perf run would be useful too.

@bors
Copy link
Contributor

bors commented Jan 3, 2024

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

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 18, 2024

Ping for a new perf run.

@Kobzol
Copy link
Contributor

Kobzol commented Jan 18, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
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"><b>Before</b></th><td colspan="2"><b>After</b></th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 <b>clap</b>:check</td><td align="right">1.6189s</td><td align="right">1.6129s</td><td align="right"> -0.37%</td></tr><tr><td>🟣 <b>hyper</b>:check</td><td align="right">0.2353s</td><td align="right">0.2337s</td><td align="right"> -0.67%</td></tr><tr><td>🟣 <b>regex</b>:check</td><td align="right">0.9344s</td><td align="right">0.9289s</td><td align="right"> -0.59%</td></tr><tr><td>🟣 <b>syn</b>:check</td><td align="right">1.4693s</td><td align="right">1.4652s</td><td align="right"> -0.28%</td></tr><tr><td>🟣 <b>syntex_syntax</b>: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
Copy link
Contributor

bors commented Jan 18, 2024

⌛ Trying commit ca6f296 with merge 7e653fc...

@bors
Copy link
Contributor

bors commented Jan 18, 2024

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

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%)

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jan 18, 2024
@bors
Copy link
Contributor

bors commented Apr 30, 2024

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

@JohnCSimon
Copy link
Member

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

@bors
Copy link
Contributor

bors commented Jul 29, 2024

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

@alex-semenyuk
Copy link
Member

@Zoxc
Form wg-triage. Closed this PR due to inactivity. Feel free to reopen or raised new one. Thanks for your efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.