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.

Sign up for GitHub

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

Jump to bottom

Remove PartialOrd, Ord from LocalDefId #90408

Merged
merged 3 commits into from
Dec 23, 2021
Merged

Remove PartialOrd, Ord from LocalDefId #90408

merged 3 commits into from
Dec 23, 2021

Conversation

pierwill
Copy link
Member

@pierwill pierwill commented Oct 29, 2021 •
edited
Loading

Part of work on #90317.

@rust-highfive
Copy link
Collaborator

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

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.

compiler/rustc_hir/src/definitions.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/definitions.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir_id.rs Outdated Show resolved Hide resolved
compiler/rustc_span/src/span_encoding.rs Outdated Show resolved Hide resolved
@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
Member Author

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

compiler/rustc_span/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_data_structures/src/graph/vec_graph/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_span/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir_id.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pierwill pierwill requested a review from cjgillot November 2, 2021 23:08
@bors
Copy link
Contributor

bors commented Dec 22, 2021

📌 Commit e6ff0ba has been approved by cjgillot

@bors
Copy link
Contributor

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
Contributor

bors commented Dec 22, 2021

⌛ Testing commit e6ff0ba with merge e983092...

@bors
Copy link
Contributor

bors commented Dec 23, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 23, 2021
@bors bors merged commit e983092 into rust-lang:master Dec 23, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 23, 2021
This was referenced Dec 23, 2021
Closed
Merged
Closed
Merged
@rust-timer
Copy link
Collaborator

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

See previous comment here: #90408 (comment).

@rustbot label: +perf-regression-triaged

@pierwill pierwill deleted the untrack-localdefid-90317 branch January 4, 2022 16:08
@pierwill pierwill mentioned this pull request Jan 17, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2022
@matthiaskrgr
Rollup merge of rust-lang#93018 - pierwill:rm-unused-ord, r=davidtwco
73988b6 
Remove some unused `Ord` derives based on `Span`

Remove some `Ord`, `PartialOrd` derivations that rely on underlying ordering of `Span`. These ordering traits appear to be unused right now.

If we're going to attempt to remove ordering traits from `Span` as suggested in rust-lang#90317 (comment), we might want to slowly remove code that depends on this ordering (as opposed to the all-at-once approach in rust-lang#90749 and rust-lang#90408).

cc `@tmiasko` `@cjgillot`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2022
@bors
Auto merge of rust-lang#90842 - pierwill:localdefid-indexmap, r=wesle…
e7825f2 
…ywiser

Use `indexmap` to avoid sorting `LocalDefId`s

See discussion in rust-lang#90408 (comment).

Related to work on rust-lang#90317.
cuviper added a commit to cuviper/rust that referenced this pull request Mar 1, 2022
@cuviper
Restore the local filter on mono item sorting
723d334 
In `CodegenUnit::items_in_deterministic_order`, there's a comment that
only local HirIds should be taken into account, but rust-lang#90408 removed the
`as_local` call that sets others to None. Restoring that check fixes the
s390x hangs seen in [RHBZ 2058803].

[RHBZ 2058803]: https://bugzilla.redhat.com/show_bug.cgi?id=2058803
@cuviper cuviper mentioned this pull request Mar 1, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 2, 2022
@Dylan-DPC
Rollup merge of rust-lang#94505 - cuviper:mono-item-sort-local, r=mic…
7588705 
…haelwoerister,davidtwco

Restore the local filter on mono item sorting

In `CodegenUnit::items_in_deterministic_order`, there's a comment that
only local HirIds should be taken into account, but rust-lang#90408 removed the
`as_local` call that sets others to None. Restoring that check fixes the
s390x hangs seen in [RHBZ 2058803].

[RHBZ 2058803]: https://bugzilla.redhat.com/show_bug.cgi?id=2058803
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 3, 2022
@Dylan-DPC
Rollup merge of rust-lang#94505 - cuviper:mono-item-sort-local, r=mic…
3e6abf0 
…haelwoerister,davidtwco

Restore the local filter on mono item sorting

In `CodegenUnit::items_in_deterministic_order`, there's a comment that
only local HirIds should be taken into account, but rust-lang#90408 removed the
`as_local` call that sets others to None. Restoring that check fixes the
s390x hangs seen in [RHBZ 2058803].

[RHBZ 2058803]: https://bugzilla.redhat.com/show_bug.cgi?id=2058803
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.