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

Revert "Merge CrateDisambiguator into StableCrateId" #85891

Merged
merged 4 commits into from
Jun 7, 2021

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jun 1, 2021

This reverts #85804

@rust-highfive
Copy link
Collaborator

r? @varkor

(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 Jun 1, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Jun 1, 2021

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

bors commented Jun 1, 2021

⌛ Trying commit 233cb5257afb4a8f3fa9e9e811acd7df9618560b with merge 6b2fbdd1b6592ef0db81db389dc81360ed9cc848...

@bors
Copy link
Contributor

bors commented Jun 1, 2021

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

@rust-timer
Copy link
Collaborator

Queued 6b2fbdd1b6592ef0db81db389dc81360ed9cc848 with parent 1160cf8, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (6b2fbdd1b6592ef0db81db389dc81360ed9cc848): 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 Jun 1, 2021
@varkor
Copy link
Member

varkor commented Jun 6, 2021

Seems like a perf improvement. Going to leave to r? @Mark-Simulacrum for the decision.

@Mark-Simulacrum
Copy link
Member

r=me, unfortunately looks like this has a bunch of merge conflicts now

@bjorn3 bjorn3 force-pushed the revert_merge_crate_disambiguator branch from 233cb52 to 8176ab8 Compare June 7, 2021 08:38
@bjorn3
Copy link
Member Author

bjorn3 commented Jun 7, 2021

Rebased

@bjorn3
Copy link
Member Author

bjorn3 commented Jun 7, 2021

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jun 7, 2021

📌 Commit 8176ab8 has been approved by Mark-Simulacrum

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

bors commented Jun 7, 2021

⌛ Testing commit 8176ab8 with merge 2312ff1...

@bors
Copy link
Contributor

bors commented Jun 7, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 2312ff1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 7, 2021
@Mark-Simulacrum
Copy link
Member

It doesn't seem like reverting this actually recovered performance -- this PR is a regression just like the merge run on the original was.

@bjorn3 my guess is that maybe reverting the revert is a good idea but I noted you suggested some possible performance improvements on the original diff (#85804 (review)) which might be worth considering. Ultimately seems like other PRs landing in between are likely causing problems with measuring and comparing this PR.

@bjorn3
Copy link
Member Author

bjorn3 commented Jun 8, 2021

Revert in #86143. I will wait with perf improvements until #85834 is merged as it introduces a method that is needed for some of the possible perf improvements.

@bjorn3 bjorn3 deleted the revert_merge_crate_disambiguator branch June 8, 2021 16:40
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 6, 2021
…mbiguator, r=michaelwoerister

Reland "Merge CrateDisambiguator into StableCrateId"

Reverts rust-lang#85891 as this revert of rust-lang#85804 made perf even worse.

r? `@Mark-Simulacrum`
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.

7 participants