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

foreign_modules query hash table lookups #78448

Merged
merged 2 commits into from
Nov 3, 2020

Conversation

rylev
Copy link
Member

@rylev rylev commented Oct 27, 2020

When compiling a large monolithic crate we're seeing huge times in the foreign_modules query due to repeated iteration over foreign modules (in order to find a module by its id). This implements hash table lookups so that which massively reduces time spent in that query in this particular case. We'll need to see if the overhead of creating the hash table has a negative impact on performance in more normal compilation scenarios.

I'm working with @wesleywiser on this.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 27, 2020
@Mark-Simulacrum
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 27, 2020

⌛ Trying commit 69dc981 with merge d0a0399ed1260c2db19783ec7bc6f5bd5b750138...

@bors
Copy link
Contributor

bors commented Oct 27, 2020

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

@rust-timer
Copy link
Collaborator

Queued d0a0399ed1260c2db19783ec7bc6f5bd5b750138 with parent 2a71e45, future comparison URL.

@petrochenkov petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (d0a0399ed1260c2db19783ec7bc6f5bd5b750138): 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

@petrochenkov petrochenkov 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 Oct 28, 2020
@rylev rylev marked this pull request as ready for review October 28, 2020 10:52
@Mark-Simulacrum
Copy link
Member

@rylev judging by those results, perf doesn't have a test case similar to the one you are benchmarking on. Would you be up for adding that? Feel free to ping me on #t-compiler/performance in Zulip and I can help guide you on how to do so.

(I guess the alternative is that this has no effect on compile times, but seems like that's likely not true?)

@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 29, 2020
@wesleywiser
Copy link
Member

r? @wesleywiser

Waiting on rust-lang/rustc-perf#789 so we can do a perf run showing the improvement.

@rylev
Copy link
Member Author

rylev commented Nov 2, 2020

@wesleywiser rust-lang/rustc-perf#789 has been merged so we should be able to get perf numbers on this now.

@wesleywiser
Copy link
Member

Awesome!

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@wesleywiser
Copy link
Member

Excellent!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 2, 2020

📌 Commit 81444b2 has been approved by wesleywiser

@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 Nov 2, 2020
@bors
Copy link
Contributor

bors commented Nov 2, 2020

⌛ Testing commit 81444b2 with merge 437651c85d101488f3d14d4e581b5d85ae725cda...

@bors
Copy link
Contributor

bors commented Nov 2, 2020

💔 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 Nov 2, 2020
@wesleywiser
Copy link
Member

@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 Nov 2, 2020
@bors
Copy link
Contributor

bors commented Nov 2, 2020

⌛ Testing commit 81444b2 with merge d4264061164e7615024245a27e33126f4b2421ea...

@bors
Copy link
Contributor

bors commented Nov 2, 2020

💔 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 Nov 2, 2020
@wesleywiser
Copy link
Member

@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 Nov 3, 2020
@bors
Copy link
Contributor

bors commented Nov 3, 2020

⌛ Testing commit 81444b2 with merge 7b5a9e9...

@bors
Copy link
Contributor

bors commented Nov 3, 2020

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 7b5a9e9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 3, 2020
@bors bors merged commit 7b5a9e9 into rust-lang:master Nov 3, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 3, 2020
@Mark-Simulacrum
Copy link
Member

As expected, a 95% win on instruction counts for the newly added externs stress test (neutral otherwise); essentially 1-1 matched by the 90% wall time reduction for the same crate.

@rylev rylev deleted the cache-foreign_modules branch November 3, 2020 21:52
@rylev
Copy link
Member Author

rylev commented Nov 5, 2020

Now that this is on nightly I tested this on the code base which prompted me to investigate this issue. On current stable (1.47.0), the crate compiles in 2min31s and on nightly (f2bbdd0 2020-11-04) it takes 58s - a more than 60% speed up 🎉!

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.

9 participants