Skip to content

Use indexmap to avoid sorting LocalDefIds - #90842

Merged
bors merged 1 commit into
rust-lang:masterfrom
pierwill:localdefid-indexmap
Jan 25, 2022
Merged

Use indexmap to avoid sorting LocalDefIds#90842
bors merged 1 commit into
rust-lang:masterfrom
pierwill:localdefid-indexmap

Conversation

@pierwill

@pierwill pierwill commented Nov 12, 2021

Copy link
Copy Markdown
Contributor

See discussion in #90408 (comment).

Related to work on #90317.

@rust-highfive

ghost commented Nov 12, 2021

Copy link
Copy Markdown
Contributor

r? @wesleywiser

(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 Nov 12, 2021
@pierwill pierwill mentioned this pull request Nov 12, 2021
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cuviper

ghost commented Nov 13, 2021 •
edited by cuviper
Loading

Copy link
Copy Markdown
Member

FWIW, indexmap does have a feature to support the normal rayon traits, but not the forked rustc-rayon.

@pierwill
pierwill marked this pull request as draft November 14, 2021 00:56
@pierwill

ghost commented Nov 14, 2021 •
edited by pierwill
Loading

Copy link
Copy Markdown
Contributor Author

FWIW, indexmap does have a feature to support the normal rayon traits, but not the forked rustc-rayon.

@cuviper Would we need to fork indexmap in order to wire it up to rustc-rayon? (Probably not what we want.) Or might there be a way to accomplish this on the rustc side?

Right now, I wouldn't say this is critical to fixing #90317. But it seems like something we'd want eventually.

@joshtriplett

ghost commented Nov 15, 2021

Copy link
Copy Markdown
Member

cc @nnethercote

Long term, I think we want to resolve the rustc-rayon vs rayon fork, which would address the problem. (For instance, we could add an unstable feature flag to rayon to enable the features rustc needs.)

@cuviper

ghost commented Nov 15, 2021

Copy link
Copy Markdown
Member

(For instance, we could add an unstable feature flag to rayon to enable the features rustc needs.)

Hmm... For a while we had --cfg rayon_unstable (not a cargo feature) that enabled support for the experimental rayon-futures crate, but we removed that in rayon-rs/rayon#716. I don't feel thrilled about heading back in that direction, but it probably would be better than letting this fork continue forever.

@cjgillot cjgillot self-assigned this Nov 27, 2021
@pierwill

ghost commented Dec 9, 2021 •
edited by pierwill
Loading

Copy link
Copy Markdown
Contributor Author

@rustbot label -S-waiting-on-review

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 9, 2021
@pierwill
pierwill force-pushed the localdefid-indexmap branch from 57a0486 to e55aa71 Compare December 9, 2021 19:06
@pierwill
pierwill marked this pull request as ready for review December 9, 2021 19:24
@pierwill
pierwill force-pushed the localdefid-indexmap branch from e55aa71 to 05aafb1 Compare December 9, 2021 19:27
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cuviper cuviper mentioned this pull request Dec 9, 2021
Merged
@pierwill
pierwill marked this pull request as draft December 10, 2021 16:09
@camelid camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 13, 2021
@bors

ghost commented Dec 16, 2021

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #86986) made this pull request unmergeable. Please resolve the merge conflicts.

@pierwill
pierwill force-pushed the localdefid-indexmap branch from 05aafb1 to 0a1e810 Compare December 16, 2021 05:26
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 16, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pierwill
pierwill force-pushed the localdefid-indexmap branch from 0a1e810 to d43c9b4 Compare December 22, 2021 16:12
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pierwill pierwill changed the title Use IndexSet and IndexMap to avoid sorting LocalDefIds Use indexmap to avoid sorting LocalDefIds Dec 22, 2021
@bors

ghost commented Dec 23, 2021

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #90408) made this pull request unmergeable. Please resolve the merge conflicts.

@pierwill
pierwill force-pushed the localdefid-indexmap branch from d43c9b4 to 61adcb5 Compare December 23, 2021 15:13
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pierwill

ghost commented Jan 23, 2022 •
edited by pierwill
Loading

Copy link
Copy Markdown
Contributor Author

@wesleywiser 👍 Conflicts addressed. @rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2022
@wesleywiser

ghost commented Jan 23, 2022

Copy link
Copy Markdown
Member

@bors r+

@bors

ghost commented Jan 23, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 4f89224 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 Jan 23, 2022
@bors

ghost commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 4f89224 with merge 797ce2dd6f5bc5958dafe4b91c5217ea9e01dc9e...

@bors

ghost commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

💥 Test timed out

@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 Jan 24, 2022
@rust-log-analyzer

ghost commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@wesleywiser

ghost commented Jan 24, 2022

Copy link
Copy Markdown
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 Jan 24, 2022
@bors

ghost commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 4f89224 with merge 50b62b69c78143416d9d13bcecccc1dc2de2c0e4...

@bors

ghost commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

💥 Test timed out

@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 Jan 24, 2022
@wesleywiser

ghost commented Jan 24, 2022

Copy link
Copy Markdown
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 Jan 24, 2022
@bors

ghost commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 4f89224 with merge e7825f2...

@rust-log-analyzer

ghost commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors

ghost commented Jan 25, 2022

Copy link
Copy Markdown
Collaborator

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 25, 2022
@rust-timer

ghost commented Jan 25, 2022

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (e7825f2): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -1.1% on incr-unchanged builds of ctfe-stress-4 check)

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

@rustbot label: -perf-regression

@klensy klensy mentioned this pull request Feb 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cuviper cuviper cuviper left review comments
@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

@cjgillot cjgillot

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.60.0

Development

Successfully merging this pull request may close these issues.