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

Use a bitset instead of a hash map in HIR ID validator #98841

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Jul 3, 2022

The hashset insertion was slightly hot in incr patched runs, but it seems unnecessary to use a hashset here, as it just checks that a dense set of N integers was seen.

This PR does two things:

  1. Enables the validation only if debug_assertions is enabled (so it's disabled in fully optimized builds now).
  2. Uses a bitset instead of a hashmap for the validation, which speeds it up considerably when it is enabled.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 3, 2022
@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2022
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 3, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

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

bors commented Jul 3, 2022

⌛ Trying commit 643f95505e4c72f631fd5b58e15c9302cfaf86dd with merge b371755aa596a2e3bb87d606d36d30cda43f8cc2...

@Mark-Simulacrum
Copy link
Member

Might also be worth trying an IntervalSet if we expect mostly contiguous insertion (I don't know if we do).

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 3, 2022

💔 Test failed - checks-actions

@bors bors 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 Jul 3, 2022
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 3, 2022

I checked the IDs and its usually contiguous in chunks, based on the visit order. So something like 14, 15, 16, 17, 8, 9, 10, 11 etc. I'll try once we see what happens with the BitSet.

@cjgillot
Copy link
Contributor

cjgillot commented Jul 3, 2022

Should we just gate all this code on debug_assertions? All it does is ICE in case of lowering bugs.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 3, 2022

Yeah I was also thinking that. It's just a sanity check (AFAIK), but consumes ~1% instructions for some incr-patched builds.

@Kobzol Kobzol force-pushed the hir-validator-bitset branch from 643f955 to 2a57e5e Compare July 4, 2022 06:30
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 4, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Jul 4, 2022

⌛ Trying commit 2a57e5e with merge 78eff9d482d3030a75548def878b6cf05c760e7e...

@bors
Copy link
Contributor

bors commented Jul 4, 2022

☀️ Try build successful - checks-actions
Build commit: 78eff9d482d3030a75548def878b6cf05c760e7e (78eff9d482d3030a75548def878b6cf05c760e7e)

@rust-timer
Copy link
Collaborator

Queued 78eff9d482d3030a75548def878b6cf05c760e7e with parent d46c728, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (78eff9d482d3030a75548def878b6cf05c760e7e): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-0.5% -0.9% 85
Improvements 🎉
(secondary)
-0.6% -1.2% 45
All 😿🎉 (primary) -0.5% -0.9% 85

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-2.5% -2.9% 2
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
2.5% 2.5% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 2.5% 2.5% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 4, 2022
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 4, 2022

Nice. Let's see how much it improves if we turn it off completely.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

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

bors commented Jul 4, 2022

⌛ Trying commit 928c172 with merge 032a2b3e8984e4f00fd8530ba03b516eb07325fa...

@bors
Copy link
Contributor

bors commented Jul 4, 2022

☀️ Try build successful - checks-actions
Build commit: 032a2b3e8984e4f00fd8530ba03b516eb07325fa (032a2b3e8984e4f00fd8530ba03b516eb07325fa)

@rust-timer
Copy link
Collaborator

Queued 032a2b3e8984e4f00fd8530ba03b516eb07325fa with parent 9c9ae85, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (032a2b3e8984e4f00fd8530ba03b516eb07325fa): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-0.7% -1.4% 119
Improvements 🎉
(secondary)
-0.8% -2.0% 78
All 😿🎉 (primary) -0.7% -1.4% 119

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
4.8% 5.2% 2
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-3.0% -3.0% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-2.6% -4.0% 5
Improvements 🎉
(secondary)
-2.5% -2.8% 2
All 😿🎉 (primary) -2.6% -4.0% 5

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 5, 2022
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 5, 2022

r? @cjgillot

@rust-highfive rust-highfive assigned cjgillot and unassigned wesleywiser Jul 5, 2022
@cjgillot
Copy link
Contributor

cjgillot commented Jul 5, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jul 5, 2022

📌 Commit 928c172 has been approved by cjgillot

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 5, 2022
@bors
Copy link
Contributor

bors commented Jul 7, 2022

⌛ Testing commit 928c172 with merge c461f7a...

@bors
Copy link
Contributor

bors commented Jul 7, 2022

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing c461f7a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 7, 2022
@bors bors merged commit c461f7a into rust-lang:master Jul 7, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 7, 2022
@Kobzol Kobzol deleted the hir-validator-bitset branch July 7, 2022 10:30
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c461f7a): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-0.8% -1.7% 123
Improvements 🎉
(secondary)
-0.8% -2.0% 82
All 😿🎉 (primary) -0.8% -1.7% 123

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
3.0% 3.8% 3
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-3.2% -3.2% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-2.2% -2.3% 3
Improvements 🎉
(secondary)
-2.2% -2.2% 1
All 😿🎉 (primary) -2.2% -2.3% 3

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@Mark-Simulacrum
Copy link
Member

@Kobzol for posterity, could you adjust the PR description to reflect that this also disables validation entirely unless debug_assertions is on (so essentially only for local rustc development)?

@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 12, 2022

@Mark-Simulacrum Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants