Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Use indexmap to avoid sorting LocalDefIds #90842

Merged
merged 1 commit into from
Jan 25, 2022
Merged

Use indexmap to avoid sorting LocalDefIds #90842

merged 1 commit into from
Jan 25, 2022

Conversation

pierwill
Copy link
Member

See discussion in #90408 (comment).

Related to work on #90317.

@rust-highfive
Copy link
Collaborator

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.

# to view
@cuviper
Copy link
Member

cuviper commented Nov 13, 2021 •
edited
Loading

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
Copy link
Member Author

pierwill commented Nov 14, 2021 •
edited
Loading

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
Copy link
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
Copy link
Member

cuviper commented Nov 15, 2021

(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
Copy link
Member Author

pierwill commented Dec 9, 2021 •
edited
Loading

@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.

# 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
Copy link
Contributor

bors commented Dec 16, 2021

☔ 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.

# 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.

# 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
Copy link
Contributor

bors commented Dec 23, 2021

☔ 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.

# to view
@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 23, 2022

📌 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
Copy link
Contributor

bors commented Jan 24, 2022

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

@bors
Copy link
Contributor

bors commented Jan 24, 2022

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

bors commented Jan 24, 2022

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

@bors
Copy link
Contributor

bors commented Jan 24, 2022

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

bors commented Jan 24, 2022

⌛ Testing commit 4f89224 with merge e7825f2...

@rust-log-analyzer
Copy link
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
Copy link
Contributor

bors commented Jan 25, 2022

☀️ 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
@bors bors merged commit e7825f2 into rust-lang:master Jan 25, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 25, 2022
@rust-timer
Copy link
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

@pierwill pierwill deleted the localdefid-indexmap branch February 7, 2022 18:52
@klensy klensy mentioned this pull request Feb 8, 2022
Merged
# for free to join this conversation on GitHub. Already have an account? # 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.