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

feat: Update hashbrown to instantiate less llvm IR #77566

Merged
merged 2 commits into from
Mar 18, 2021

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Oct 5, 2020

Includes rust-lang/hashbrown#204 and rust-lang/hashbrown#205 (not yet merged) which both serve to reduce the amount of IR generated for hashmaps.

Inspired by the llvm-lines data gathered in #76680 (cc @Julian-Wollersberger)

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Oct 5, 2020
@Marwes
Copy link
Contributor Author

Marwes commented Oct 5, 2020

Prompted by rust-lang/hashbrown#205 (comment), this could use a perf run.

@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 5, 2020

⌛ Trying commit c2c7c50ccb799451b06fb30a210bea2eb16ab059 with merge 4a35079c2dbc0f04dbb93febe7bf6f980ffb6a59...

@Marwes
Copy link
Contributor Author

Marwes commented Oct 5, 2020

Had to update the the git dependency to point to the branch as it should, and fix the branch as hashbrown's ci didn't test the raw feature which rustc uses.

@lcnr
Copy link
Contributor

lcnr commented Oct 5, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 5, 2020

⌛ Trying commit 534f867a487cbdad97a1dd97cf7552b96e87016a with merge e035000c1a2a0b031b1862e7b9b1d8f59efd7e7a...

@bors
Copy link
Contributor

bors commented Oct 5, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: e035000c1a2a0b031b1862e7b9b1d8f59efd7e7a (e035000c1a2a0b031b1862e7b9b1d8f59efd7e7a)

@rust-timer
Copy link
Collaborator

Queued e035000c1a2a0b031b1862e7b9b1d8f59efd7e7a with parent 62bfcfd, future comparison URL.

@jyn514 jyn514 added I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 5, 2020
@Julian-Wollersberger
Copy link
Contributor

Nice work! Here are some measurements of this PR.
(I recently wrote an entry in rustc-dev-guide how to do these measurements with a rustc debug build, to safe time.)

Sum of hashbrown lines before: 1 615 640
Sum of hashbrown lines after: 1 077 708
That's -537 932 lines! (-33%)

The total difference is 589 850 lines (-2.9%).

Here I filtered for hashbrown:

# Before:
  Lines            Copies         Function name
  -----            ------         -------------
  20188433 (100%)  596578 (100%)  (TOTAL)
    188221 (0.9%)     781 (0.1%)  hashbrown::raw::RawTable<T>::rehash_in_place
    139799 (0.7%)     781 (0.1%)  hashbrown::raw::RawTable<T>::resize
     89815 (0.4%)    1562 (0.3%)  hashbrown::raw::RawTable<T>::rehash_in_place::{{closure}}
     85074 (0.4%)    1289 (0.2%)  hashbrown::raw::RawTable<T>::find
     73140 (0.4%)     636 (0.1%)  hashbrown::raw::RawTable<T>::fallible_with_capacity

# After:
  19598583 (100%)  583746 (100%)  (TOTAL)
     89676 (0.5%)     636 (0.1%)  hashbrown::raw::RawTable<T>::rehash_in_place
     85860 (0.4%)     636 (0.1%)  hashbrown::raw::RawTable<T>::resize
     66576 (0.3%)     552 (0.1%)  hashbrown::raw::RawTable<T>::insert
     63210 (0.3%)     957 (0.2%)  hashbrown::raw::RawTable<T>::find
     58317 (0.3%)     536 (0.1%)  hashbrown::map::HashMap<K,V,S>::insert
     53424 (0.3%)     636 (0.1%)  hashbrown::raw::RawTableInner::fallible_with_capacity

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (e035000c1a2a0b031b1862e7b9b1d8f59efd7e7a): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@Marwes
Copy link
Contributor Author

Marwes commented Oct 5, 2020

Improvements in compiling rustc itself but there does seem to be a slight loss in performance when using the hashmap itself (I assume). I suspect it is the iteration code that got the slowdown so I will see if I can't tweak it a bit.

@Marwes Marwes force-pushed the smaller_hashmap branch 2 times, most recently from 7b92a13 to bf69d26 Compare October 6, 2020 14:35
@Marwes
Copy link
Contributor Author

Marwes commented Oct 6, 2020

Perf should be more stable now, another perf would be nice!

Not sure what is going on with the debug info providers yet, the changes in hashbrown forces an extra .table indirection but I don't seem to have added that correctly.

@lcnr
Copy link
Contributor

lcnr commented Oct 6, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 6, 2020

⌛ Trying commit bf69d26632b6015f2b390f69cb2fc018341c571e with merge ce08d801f6fb1e045de36dbd8733c13eda8c0e37...

@bors
Copy link
Contributor

bors commented Oct 6, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: ce08d801f6fb1e045de36dbd8733c13eda8c0e37 (ce08d801f6fb1e045de36dbd8733c13eda8c0e37)

@rust-timer
Copy link
Collaborator

Queued ce08d801f6fb1e045de36dbd8733c13eda8c0e37 with parent 08e2d46, future comparison URL.

@bors
Copy link
Contributor

bors commented Mar 17, 2021

⌛ Testing commit 81c9a02 with merge 547d71f63137d7c0fc97a5b627596cbfec5c3720...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
 19  516M   19  102M    0     0  24.4M      0  0:00:21  0:00:04  0:00:17 24.4M
 20  516M   20  105M    0     0  24.4M      0  0:00:21  0:00:04  0:00:17 24.7M
curl: (56) OpenSSL SSL_read: Connection was reset, errno 10054

gzip: stdin: unexpected end of file
tar: Unexpected EOF in archive
tar: Unexpected EOF in archive
tar: Error is not recoverable: exiting now
##[error]Process completed with exit code 2.
[command]"C:\Program Files\Git\bin\git.exe" version
git version 2.30.1.windows.1
[command]"C:\Program Files\Git\bin\git.exe" config --local --name-only --get-regexp core\.sshCommand
[command]"C:\Program Files\Git\bin\git.exe" submodule foreach --recursive "git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :"
[command]"C:\Program Files\Git\bin\git.exe" submodule foreach --recursive "git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :"
[command]"C:\Program Files\Git\bin\git.exe" config --local --name-only --get-regexp http\.https\:\/\/github\.com\/\.extraheader
http.https://github.com/.extraheader
[command]"C:\Program Files\Git\bin\git.exe" config --local --unset-all http.https://github.com/.extraheader
[command]"C:\Program Files\Git\bin\git.exe" submodule foreach --recursive "git config --local --name-only --get-regexp 'http\.https\:\/\/github\.com\/\.extraheader' && git config --local --unset-all 'http.https://github.com/.extraheader' || :"
Cleaning up orphan processes
Terminate orphan process: pid (6704) (node)
Terminate orphan process: pid (5984) (python)
Terminate orphan process: pid (1236) (vctip)

@bors
Copy link
Contributor

bors commented Mar 17, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 17, 2021
@Amanieu
Copy link
Member

Amanieu commented Mar 18, 2021

@bors retry

@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. labels Mar 18, 2021
@bors
Copy link
Contributor

bors commented Mar 18, 2021

⌛ Testing commit 81c9a02 with merge 0464f63...

@bors
Copy link
Contributor

bors commented Mar 18, 2021

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 0464f63 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 18, 2021
@bors bors merged commit 0464f63 into rust-lang:master Mar 18, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 18, 2021
@Marwes Marwes deleted the smaller_hashmap branch March 18, 2021 14:43
@rylev
Copy link
Member

rylev commented Mar 23, 2021

@Marwes @Amanieu As you saw in your perf runs, this regresses performance slightly in the eval_to_const_value_raw query in the ctfe-stress-4-debug benchmark. We might want to look into that and see if we can figure out why that might be. I'm unfamiliar with that query though. It was mentioned that iteration performance might be impacted negatively by this upgrade. Any thoughts on this?

@ghost
Copy link

ghost commented Mar 23, 2021

The iteration thing was a red herring, so I can't quite say why it would regress in that particular case.

artemmukhin added a commit to artemmukhin/rust that referenced this pull request Apr 6, 2021
The pretty-printer was broken in rust-lang#77566
after updating hashbrown to 0.11.0.
Note that the corresponding GDB pretty-printer was updated properly.
artemmukhin added a commit to intellij-rust/intellij-rust that referenced this pull request Apr 6, 2021
bors bot added a commit to intellij-rust/intellij-rust that referenced this pull request Apr 6, 2021
7059: Update HashMap/HashSet pretty-printers to Rust 1.52 r=Undin a=ortem

The corresponding PRs in rustc:
rust-lang/rust#77566
rust-lang/rust#83920

Fixes #7045

changelog: Update LLDB/GDB pretty-printers to render `HashMap`/`HashSet` on Rust 1.52 or higher

Co-authored-by: ortem <ortem00@gmail.com>
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 6, 2021
…er-1.52, r=pnkfelix

Fix HashMap/HashSet LLDB pretty-printer after hashbrown 0.11.0

The pretty-printer was broken in rust-lang#77566 after updating hashbrown to 0.11.0.
Note that the corresponding GDB pretty-printer was updated properly.

Fixes rust-lang#83891
Undin pushed a commit to intellij-rust/intellij-rust that referenced this pull request Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. 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.