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

Record LocalDefId in HIR nodes instead of a side table #104170

Merged
merged 8 commits into from
Nov 17, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Nov 8, 2022

This is part of an attempt to remove the HirId -> LocalDefId table from HIR.
This attempt is a prerequisite to creation of LocalDefId after HIR lowering (#96840), by controlling how def_id information is accessed.

This first part adds the information to HIR nodes themselves instead of a table.
The second part is #103902
The third part will be to make hir::Visitor::visit_fn take a LocalDefId as last parameter.
The fourth part will be to completely remove the side table.

@rustbot
Copy link
Collaborator

rustbot commented Nov 8, 2022

r? @fee1-dead

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 8, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 8, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@cjgillot
Copy link
Contributor Author

cjgillot commented Nov 8, 2022

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 8, 2022
@bors
Copy link
Contributor

bors commented Nov 8, 2022

⌛ Trying commit 4a10ad4401335da257868d5f16acd69554010ff6 with merge 4455c124dba8daf716e860e09ff6f1900be13c57...

@bors
Copy link
Contributor

bors commented Nov 9, 2022

☀️ Try build successful - checks-actions
Build commit: 4455c124dba8daf716e860e09ff6f1900be13c57 (4455c124dba8daf716e860e09ff6f1900be13c57)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4455c124dba8daf716e860e09ff6f1900be13c57): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.3% [1.3%, 1.3%] 2
Improvements ✅
(primary)
-0.2% [-0.2%, -0.2%] 2
Improvements ✅
(secondary)
-1.1% [-2.2%, -0.7%] 12
All ❌✅ (primary) -0.2% [-0.2%, -0.2%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.7% [1.3%, 2.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.3% [-7.4%, -2.7%] 4
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.2% [1.2%, 1.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.2% [1.2%, 1.2%] 1

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 9, 2022
@fee1-dead
Copy link
Member

Could we add the LocalDefId to HirId as a field?

@cjgillot
Copy link
Contributor Author

cjgillot commented Nov 9, 2022

Could we add the LocalDefId to HirId as a field?

I'd rather not. There are much fewer nodes that have a dedicated LocalDefId than nodes that have a HirId. Namely, only generic params, struct variants & fields and anonymous constants have a LocalDefId. The others only have a HirId and storing an Option<LocalDefId> would use 4 bytes for each of them.

@fee1-dead
Copy link
Member

the regression on externs should be inlining noise. See #102795 (comment)

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Nov 13, 2022
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The memory did regress a bit but not too much.

@fee1-dead
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 14, 2022

📌 Commit df5c11a has been approved by fee1-dead

It is now in the queue for this repository.

@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 Nov 14, 2022
@Manishearth
Copy link
Member

@bors p=1

going to close the tree for non-nevers for a while so they can drain out

@bors
Copy link
Contributor

bors commented Nov 17, 2022

⌛ Testing commit df5c11a with merge 7c75fe4...

@bors
Copy link
Contributor

bors commented Nov 17, 2022

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing 7c75fe4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 17, 2022
@bors bors merged commit 7c75fe4 into rust-lang:master Nov 17, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 17, 2022
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Nov 17, 2022
Record `LocalDefId` in HIR nodes instead of a side table

This is part of an attempt to remove the `HirId -> LocalDefId` table from HIR.
This attempt is a prerequisite to creation of `LocalDefId` after HIR lowering (rust-lang#96840), by controlling how `def_id` information is accessed.

This first part adds the information to HIR nodes themselves instead of a table.
The second part is rust-lang#103902
The third part will be to make `hir::Visitor::visit_fn` take a `LocalDefId` as last parameter.
The fourth part will be to completely remove the side table.
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7c75fe4): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.3% [-0.3%, -0.2%] 4
Improvements ✅
(secondary)
-1.1% [-2.3%, -0.6%] 12
All ❌✅ (primary) -0.3% [-0.3%, -0.2%] 4

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
2.6% [2.1%, 3.0%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.4%, 0.4%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.7% [-0.7%, -0.7%] 1
Improvements ✅
(secondary)
-3.2% [-3.6%, -2.7%] 3
All ❌✅ (primary) -0.7% [-0.7%, -0.7%] 1

@rustbot rustbot removed the perf-regression Performance regression. label Nov 17, 2022
@cjgillot cjgillot deleted the hir-def-id branch November 17, 2022 15:59
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 21, 2022
Record `LocalDefId` in HIR nodes instead of a side table

This is part of an attempt to remove the `HirId -> LocalDefId` table from HIR.
This attempt is a prerequisite to creation of `LocalDefId` after HIR lowering (rust-lang#96840), by controlling how `def_id` information is accessed.

This first part adds the information to HIR nodes themselves instead of a table.
The second part is rust-lang#103902
The third part will be to make `hir::Visitor::visit_fn` take a `LocalDefId` as last parameter.
The fourth part will be to completely remove the side table.
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Record `LocalDefId` in HIR nodes instead of a side table

This is part of an attempt to remove the `HirId -> LocalDefId` table from HIR.
This attempt is a prerequisite to creation of `LocalDefId` after HIR lowering (rust-lang#96840), by controlling how `def_id` information is accessed.

This first part adds the information to HIR nodes themselves instead of a table.
The second part is rust-lang#103902
The third part will be to make `hir::Visitor::visit_fn` take a `LocalDefId` as last parameter.
The fourth part will be to completely remove the side table.
bors-servo added a commit to servo/servo that referenced this pull request Feb 2, 2023
Upgrade the Rust toolchain to 'nightly-2023-02-01'

<!-- Please describe your changes on the following line: -->

This change should address the failing nightly [rustc test jobs](https://github.com/servo/servo/actions/workflows/nightly-rust.yml)

For reference, these are the [relevant](rust-lang/rust#107206) [PRs](rust-lang/rust#104170) in rustc that I could find.

Signed-off-by: Mukilan Thiyagarajan <mukilanthiagarajan@gmail.com>

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because there are existing unit tests for script_plugins that do pass.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit to servo/servo that referenced this pull request Feb 2, 2023
Upgrade the Rust toolchain to 'nightly-2023-02-01'

<!-- Please describe your changes on the following line: -->

This change should address the failing nightly [rustc test jobs](https://github.com/servo/servo/actions/workflows/nightly-rust.yml)

For reference, these are the [relevant](rust-lang/rust#107206) [PRs](rust-lang/rust#104170) in rustc that I could find.

Signed-off-by: Mukilan Thiyagarajan <mukilanthiagarajan@gmail.com>

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because there are existing unit tests for script_plugins that do pass.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
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. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants