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

Avoid nondeterminism in trimmed_def_paths #89408

Merged
merged 1 commit into from
Oct 2, 2021

Conversation

Mark-Simulacrum
Copy link
Member

Previously this query depended on the global interning order of Symbols, which
meant that irrelevant changes could influence the query and cause
recompilations. This commit ensures that the return set is stable and will not
be affected by the global order by deterministically (in lexicographic order)
choosing a name to use if there are multiple names for a single DefId.

This should fix the cause of the regressions in #83343.

Previously this query depended on the global interning order of Symbols, which
meant that irrelevant changes could influence the query and cause
recompilations. This commit ensures that the return set is stable and will not
be affected by the global order by deterministically (in lexicographic order)
choosing a name to use if there are multiple names for a single DefId.
@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 Sep 30, 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 Sep 30, 2021
@bors
Copy link
Contributor

bors commented Sep 30, 2021

⌛ Trying commit 56fcf07 with merge bde948fea1e2e10b64d5edf9ca9c1aba01f2da10...

@Mark-Simulacrum
Copy link
Member Author

(Nondeterminism is maybe not the right name -- a specific compilation should always be the same. But "irrelevant" changes can cause the behavior of this query to differ, which seems not great).

@bors
Copy link
Contributor

bors commented Sep 30, 2021

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

@rust-timer
Copy link
Collaborator

Queued bde948fea1e2e10b64d5edf9ca9c1aba01f2da10 with parent 6dc08b9, future comparison URL.

@tgnottingham
Copy link
Contributor

Seems like a good idea.

Outside the scope of this, but it might be nice if trimmed paths in error messages used the most appropriate name when there are multiple possibilities, taking into account any use ... as ... as applicable. That may get tricky if the error message involves multiple locations and they use different names for the same def id though.

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bde948fea1e2e10b64d5edf9ca9c1aba01f2da10): comparison url.

Summary: This benchmark run did not return any relevant changes.

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 led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@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 Sep 30, 2021
@rylev
Copy link
Member

rylev commented Oct 1, 2021

This indeed seems to fix the performance regressions we saw previously! 🎉

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 1, 2021

📌 Commit 56fcf07 has been approved by petrochenkov

@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 Oct 1, 2021
@bors
Copy link
Contributor

bors commented Oct 2, 2021

⌛ Testing commit 56fcf07 with merge edebf77...

@bors
Copy link
Contributor

bors commented Oct 2, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing edebf77 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 2, 2021
@bors bors merged commit edebf77 into rust-lang:master Oct 2, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 2, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (edebf77): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

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.

8 participants