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

Streamline try_start code #84806

Merged
merged 1 commit into from May 7, 2021
Merged

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented May 2, 2021

This shifts some branches around and avoids interleaving parallel and
non-parallel versions of the function too much.

@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 May 2, 2021
@Mark-Simulacrum
Copy link
Member Author

@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 May 2, 2021
@bors
Copy link
Contributor

bors commented May 2, 2021

⌛ Trying commit 35c2e168da494b028f3ddee7c7a4b1d8472d8273 with merge 2a30499874245ed414899e7f24a568990a4dfb9c...

@bors
Copy link
Contributor

bors commented May 2, 2021

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

@rust-timer
Copy link
Collaborator

Queued 2a30499874245ed414899e7f24a568990a4dfb9c with parent 4de7572, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2a30499874245ed414899e7f24a568990a4dfb9c): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 2, 2021
@petrochenkov
Copy link
Contributor

r? @cjgillot

Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this reduces the spaghettiness of the current code.
The bootstrap perf effect is small but interesting.
The runtime perf effect is marginally negative: do you have an idea why?

compiler/rustc_query_system/src/query/plumbing.rs Outdated Show resolved Hide resolved
compiler/rustc_query_system/src/query/plumbing.rs Outdated Show resolved Hide resolved
@cjgillot
Copy link
Contributor

cjgillot commented May 2, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 2, 2021

📌 Commit ba04d692fdb2e1ffdb6aae053c44dd8fb6841cab 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. labels May 2, 2021
@Mark-Simulacrum
Copy link
Member Author

Pushed a cleanup fix to the code, but no performance fix quite yet.

I think the performance regression is because we're generating some extra code due to needing to re-fill the slot if we're encountering a cycle error; the entry API avoided this because it never inserted into an occupied slot. It's not entirely obvious though if this is the actual cause or not, hard to say.

Looking at a possible fix, in the meantime let me @bors r-

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 2, 2021
@Mark-Simulacrum
Copy link
Member Author

@bors try @rust-timer queue

Went back to using entry API, but still seems to be a compile-time win, just only ~0.5% instruction count wise. Will update PR description if the perf run comes back with no performance regression.

@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 May 2, 2021
@bors
Copy link
Contributor

bors commented May 2, 2021

⌛ Trying commit 025590ac7ba56a345e466a5c362e27251636cce3 with merge 75f6f6764205c5299fcdc2061a07476875511331...

This shifts some branches around and avoids interleaving parallel and
non-parallel versions of the function too much.
@Mark-Simulacrum
Copy link
Member Author

@rust-timer build 75f6f6764205c5299fcdc2061a07476875511331

@rust-timer
Copy link
Collaborator

Queued 75f6f6764205c5299fcdc2061a07476875511331 with parent 89ebad5, future comparison URL.

@Mark-Simulacrum Mark-Simulacrum changed the title Optimize try_start to avoid using entry API Streamline try_start code May 2, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (75f6f6764205c5299fcdc2061a07476875511331): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@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 May 2, 2021
@Mark-Simulacrum
Copy link
Member Author

Looks like it retained the win on compile-time and is a slight bit faster, even.

@Mark-Simulacrum
Copy link
Member Author

r? @cjgillot

@Mark-Simulacrum Mark-Simulacrum removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 6, 2021
@cjgillot
Copy link
Contributor

cjgillot commented May 6, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 6, 2021

📌 Commit 981135a 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. labels May 6, 2021
@bors
Copy link
Contributor

bors commented May 6, 2021

⌛ Testing commit 981135a with merge 777bb2f...

@bors
Copy link
Contributor

bors commented May 7, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 7, 2021
@bors bors merged commit 777bb2f into rust-lang:master May 7, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 7, 2021
@Mark-Simulacrum Mark-Simulacrum deleted the try-start-entry branch May 7, 2021 02:20
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants