Skip to content

Remove PartialOrd, Ord from LocalDefId#90408

Merged
bors merged 3 commits intorust-lang:masterfrom
pierwill:untrack-localdefid-90317
Dec 23, 2021
Merged

Remove PartialOrd, Ord from LocalDefId#90408
bors merged 3 commits intorust-lang:masterfrom
pierwill:untrack-localdefid-90317

Conversation

@pierwill
Copy link
Contributor

@pierwill pierwill commented Oct 29, 2021 •
edited
Loading

Part of work on #90317.

@rust-highfive
Copy link
Contributor

rust-highfive commented Oct 29, 2021

r? @jackh726

(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 Oct 29, 2021
@pierwill
Copy link
Contributor Author

pierwill commented Oct 29, 2021

r? @cjgillot

@rust-highfive rust-highfive assigned cjgillot and unassigned jackh726 Oct 29, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
cjgillot
cjgillot reviewed Oct 30, 2021
View reviewed changes
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.

Thanks for the PR @pierwill! I left a few comments.
Those trait bounds on LocalDefId are used a lot, it may be easier to handle Idx and PartialOrd/Ord separately.
Changing data structures means we will have to be careful about perf implications.

@cjgillot cjgillot 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2021
@pierwill pierwill changed the title [WIP] Remove PartialOrd, Ord, and Idx from LocalDefId [WIP] Remove PartialOrd, Ord from LocalDefId Oct 30, 2021
@pierwill
Copy link
Contributor Author

pierwill commented Oct 30, 2021

Those trait bounds on LocalDefId are used a lot, it may be easier to handle Idx and PartialOrd/Ord separately.

Thanks for reviewing, @cjgillot! I'll work on just the ordering traits for now. I also might see if DefId is easier to start with (#90317 (comment)).

@pierwill pierwill force-pushed the untrack-localdefid-90317 branch from 624198a to ae5bf6a Compare October 30, 2021 15:12
@pierwill pierwill changed the title [WIP] Remove PartialOrd, Ord from LocalDefId [WIP] Remove PartialOrd, Ord from LocalDefId Oct 30, 2021
@pierwill
Copy link
Contributor Author

pierwill commented Oct 30, 2021 •
edited
Loading

Those trait bounds on LocalDefId are used a lot, it may be easier to handle Idx and PartialOrd/Ord separately.

Looks like Idx requires Ord...

rust/compiler/rustc_index/src/vec.rs

Line 15 in 2b643e9

pub trait Idx: Copy + 'static + Ord + Debug + Hash {

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pierwill pierwill mentioned this pull request Oct 31, 2021
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 31, 2021
@matthiaskrgr
Rollup merge of rust-lang#90450 - pierwill:rm-hiridvec, r=cjgillot
f3f2b48 
Remove `rustc_hir::hir_id::HirIdVec`

See rust-lang#90408 (comment):

> IIRC, `HirIdVec` is never used, you can delete it. PR rust-lang#72015 has been abandoned.

r? `@cjgillot`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 1, 2021
@matthiaskrgr
Rollup merge of rust-lang#90450 - pierwill:rm-hiridvec, r=cjgillot
86f5b8a 
Remove `rustc_hir::hir_id::HirIdVec`

See rust-lang#90408 (comment):

> IIRC, `HirIdVec` is never used, you can delete it. PR rust-lang#72015 has been abandoned.

r? `@cjgillot`
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pierwill pierwill force-pushed the untrack-localdefid-90317 branch from 39238b3 to 8b48d70 Compare November 2, 2021 18:39
@pierwill pierwill changed the title [WIP] Remove PartialOrd, Ord from LocalDefId Remove PartialOrd, Ord from LocalDefId Nov 2, 2021
@pierwill pierwill requested a review from cjgillot November 2, 2021 18:40
@rust-log-analyzer

This comment has been minimized.

Sign in to view
cjgillot
cjgillot reviewed Nov 2, 2021
View reviewed changes
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.

Thanks @pierwill. I added a few nits. The main remark is that we'd like to remove the PartialOrd/Ord impl for HirId, since it has exactly the same issue as the impl for LocalDefId.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pierwill pierwill requested a review from cjgillot November 2, 2021 23:08
pierwill
pierwill commented Nov 2, 2021
View reviewed changes
@pierwill
Copy link
Contributor Author

pierwill commented Dec 13, 2021

This change has been failing the same Windows backtrace test as #89836. We're assessing whether to bless the test or continue digging.

@pierwill pierwill force-pushed the untrack-localdefid-90317 branch from 439c33d to 46a508b Compare December 13, 2021 16:35
pierwill added a commit to pierwill/rust that referenced this pull request Dec 13, 2021
@pierwill
Update regression test for rust-lang#87481
640eeb9 
This has been causing failures in CI here and
in rust-lang#90408. A comment there
(rust-lang#90408 (comment))
suggested blessing the output; this commit does that manually.
@pierwill
Copy link
Contributor Author

pierwill commented Dec 15, 2021

Once #89836 is merged (🤞🤞), this should be ready to go. :)

@pierwill pierwill force-pushed the untrack-localdefid-90317 branch from 46a508b to f00fd59 Compare December 16, 2021 05:20
@matthiaskrgr
Copy link
Member

matthiaskrgr commented Dec 16, 2021

Just for the record, the failure here #90408 (comment) is probably unrelated to this PR since it happened in other ci-runs as well :( : #91995 (comment)

Made a tracking ticket: #92000

@pierwill
Copy link
Contributor Author

pierwill commented Dec 16, 2021 via email

@pierwill pierwill force-pushed the untrack-localdefid-90317 branch from f00fd59 to e6ff0ba Compare December 22, 2021 16:51
@pierwill pierwill mentioned this pull request Dec 22, 2021
Closed
@bjorn3
Copy link
Member

bjorn3 commented Dec 22, 2021

@bors r=cjgillot

@bors
Copy link
Collaborator

bors commented Dec 22, 2021

📌 Commit e6ff0ba has been approved by cjgillot

@bors
Copy link
Collaborator

bors commented Dec 22, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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 Dec 22, 2021
@pierwill pierwill mentioned this pull request Dec 22, 2021
Closed
@bors
Copy link
Collaborator

bors commented Dec 22, 2021

⌛ Testing commit e6ff0ba with merge e983092...

@bors
Copy link
Collaborator

bors commented Dec 23, 2021

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

This was referenced Dec 23, 2021
Closed
Merged
Closed
Merged
@rust-timer
Copy link
Collaborator

rust-timer commented Dec 23, 2021

Finished benchmarking commit (e983092): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

  • Large improvement in instruction counts (up to -3.7% on full builds of deeply-nested-async)
  • Large regression in instruction counts (up to 2.9% on incr-unchanged builds of deeply-nested-async)

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@pierwill
Copy link
Contributor Author

pierwill commented Dec 23, 2021

See previous comment here: #90408 (comment).

@rustbot label: +perf-regression-triaged

@pierwill pierwill mentioned this pull request Jan 17, 2022
Merged
@cuviper cuviper mentioned this pull request Mar 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@cjgillot cjgillot cjgillot approved these changes

@Mark-Simulacrum Mark-Simulacrum Awaiting requested review from Mark-Simulacrum

Assignees

@cjgillot cjgillot

Labels

merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.59.0

Development

Successfully merging this pull request may close these issues.