-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
HirIdification: rework Map #59042
HirIdification: rework Map #59042
Conversation
@bors try (for perf) |
HirIdification: rework Map The next iteration of HirIdification (#57578). - remove `NodeId` from `Entry` - change `Map::map` to an `FxHashMap<HirId, Entry>` - base the `NodeId` `Map` methods on `HirId` ones (reverses the current state) - HirIdify `librustdoc` a little bit (some `NodeId` `Map` methods were converted to work on `HirId`s) The second change might have performance implications, so I'd do a perf run to be sure it's fine; it simplifies the codebase and shouldn't have an impact as long as the `Map` searches are cached (which is now possible thanks to using `HirId`s). r? @Zoxc
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
12c55fd
to
16b3081
Compare
Oof @bors try (for perf, for real this time) |
HirIdification: rework Map The next iteration of HirIdification (#57578). - remove `NodeId` from `Entry` - change `Map::map` to an `FxHashMap<HirId, Entry>` - base the `NodeId` `Map` methods on `HirId` ones (reverses the current state) - HirIdify `librustdoc` a little bit (some `NodeId` `Map` methods were converted to work on `HirId`s) The second change might have performance implications, so I'd do a perf run to be sure it's fine; it simplifies the codebase and shouldn't have an impact as long as the `Map` searches are cached (which is now possible thanks to using `HirId`s). r? @Zoxc
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☀️ Try build successful - checks-travis |
Can I get a perf run pls? |
Assuming I remember the invocation... @rust-timer build 3c13d8d |
@rust-timer build 3c13d8d |
Success: Queued 3c13d8d with parent c9f8304, comparison URL. |
@Mark-Simulacrum huh so only bare command works in the comment body? |
No, it should've responded to you somehow. I need to investigate why it didn't. |
Finished benchmarking try commit 3c13d8d |
Hmm, looks like there are some instructions and max-rss losses. @Zoxc do you think we should also give it a shot together with #57173 or that the |
I would wait until #58623 lands before trying to replace the existing node map. Until then I'd just focus on getting rid of |
Ok; can you mark this PR as blocked so it doesn't pop up during PR queue cleanups? |
Is it really necessary to wait for #58623 to land? That PR only touches libstd and doesn't really interact with this PR. The HashMap API is completely unchanged, just the implementation. |
📌 Commit 37954df has been approved by |
⌛ Testing commit 37954df with merge 49c193172ef3ae229b5c8149a67ff5e31c0d4672... |
I mean the instruction results did seem to improve in comparison to the previous perf run; did you expect greens in the incremental department? |
@ljedrz wall-time is the metric which counts, which seems pretty much as bad as before. |
Yielding priority to the stable release. @bors retry |
⌛ Testing commit 37954df with merge 04480fd5dd4945b4a4bb31e46934bff1739df63b... |
Yielding priority to the stable release, again. @bors retry |
HirIdification: rework Map The next iteration of HirIdification (#57578). - remove `NodeId` from `Entry` - change `Map::map` to an `FxHashMap<HirId, Entry>` - base the `NodeId` `Map` methods on `HirId` ones (reverses the current state) - HirIdify `librustdoc` a little bit (some `NodeId` `Map` methods were converted to work on `HirId`s) The second change might have performance implications, so I'd do a perf run to be sure it's fine; it simplifies the codebase and shouldn't have an impact as long as the `Map` searches are cached (which is now possible thanks to using `HirId`s). r? @Zoxc
☀️ Test successful - checks-travis, status-appveyor |
📣 Toolstate changed by #59042! Tested on commit 3d21124. 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). |
Tested on commit rust-lang/rust@3d21124. Direct link to PR: <rust-lang/rust#59042> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). 💔 rls on windows: test-fail → build-fail (cc @Xanewok, @rust-lang/infra). 💔 rls on linux: test-fail → build-fail (cc @Xanewok, @rust-lang/infra).
Rustup for rust-lang/rust#59042 changelog: none
Changes: ```` Rustup for rust-lang#59042 Update pulldown_cmark to 0.5 Only run AppVeyor on r+, try and the master branch Remove approx_constant known problems Suppress let_and_return if let has attributes Add test for or_fun_call macro suggestion UI test cleanup: Extract needless_range_loop tests Change "if types change" to "if you later change the type" ````
submodules: update clippy from 9897442 to 8c0e038 Should fix clippy/rls toolstate breakage Changes: ```` Rustup for #59042 Update pulldown_cmark to 0.5 Only run AppVeyor on r+, try and the master branch Remove approx_constant known problems Suppress let_and_return if let has attributes Add test for or_fun_call macro suggestion UI test cleanup: Extract needless_range_loop tests Change "if types change" to "if you later change the type" ```` r? @oli-obk
submodules: update clippy from 9897442 to 8c0e038 Should fix clippy/rls toolstate breakage Changes: ```` Rustup for #59042 Update pulldown_cmark to 0.5 Only run AppVeyor on r+, try and the master branch Remove approx_constant known problems Suppress let_and_return if let has attributes Add test for or_fun_call macro suggestion UI test cleanup: Extract needless_range_loop tests Change "if types change" to "if you later change the type" ```` r? @oli-obk
Changes: ```` Rustup for rust-lang/rust#59042 Update pulldown_cmark to 0.5 Only run AppVeyor on r+, try and the master branch Remove approx_constant known problems Suppress let_and_return if let has attributes Add test for or_fun_call macro suggestion UI test cleanup: Extract needless_range_loop tests Change "if types change" to "if you later change the type" ````
The next iteration of HirIdification (#57578).
NodeId
fromEntry
Map::map
to anFxHashMap<HirId, Entry>
NodeId
Map
methods onHirId
ones (reverses the current state)librustdoc
a little bit (someNodeId
Map
methods were converted to work onHirId
s)The second change might have performance implications, so I'd do a perf run to be sure it's fine; it simplifies the codebase and shouldn't have an impact as long as the
Map
searches are cached (which is now possible thanks to usingHirId
s).r? @Zoxc