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

Shrink hir::def::Res #101887

Merged
merged 2 commits into from
Sep 30, 2022
Merged

Shrink hir::def::Res #101887

merged 2 commits into from
Sep 30, 2022

Conversation

nnethercote
Copy link
Contributor

@rustbot rustbot added 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 Sep 16, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 16, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in need_type_info.rs

cc @lcnr

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2022
@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

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

bors commented Sep 16, 2022

⌛ Trying commit a8492962c36d63271af04d85fb20e811b045bce2 with merge 2e42f14cf381b553c44ffca6fba169be755ea436...

@petrochenkov petrochenkov self-assigned this Sep 16, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment was marked as resolved.

Sign in to view
@bors bors 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 Sep 16, 2022
@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Sep 16, 2022

⌛ Trying commit 4ca8b9fff9022c84a4bee41cacbdc20ceab35dbc with merge cc30719e15b97b59204b625423adb870a39b07ce...

@bors

This comment was marked as resolved.

Sign in to view
petrochenkov
petrochenkov reviewed Sep 16, 2022
View reviewed changes
compiler/rustc_hir/src/def.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/def.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/def.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/def.rs Show resolved Hide resolved
@petrochenkov petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 16, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nnethercote
Copy link
Contributor Author

One frustrating thing is that after this change, hir::PathSegment is 48 bytes:

pub struct PathSegment<'hir> {
    pub ident: Ident,      // 12 bytes
    pub hir_id: HirId,     // 8 bytes
    pub res: Res,          // 12 bytes
    pub args: Option<&'hir GenericArgs<'hir>>, // 8 bytes
    pub infer_args: bool,  // 1 bit of data taking up 1 byte, followed by 7 bytes of padding
}

Theoretically, infer_args could be squeezed into Res because the largest variant of Res has one byte of padding. But it would be very ugly and affect lots of code unrelated to PathSegment.

A better alternative: change args to a tagged pointer with a two bit tag. The first tag bit could represent none/some (i.e. manually doing the Option) and the second tag bit could represent infer_args.

@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Sep 16, 2022

⌛ Trying commit d48b6a9c6c0023d3a4a6fc6db6f2e2dc17d04deb with merge a276ba6fd94b4c292920ebd9bede877948cb9169...

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 17, 2022
@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2022
@petrochenkov petrochenkov 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 Sep 19, 2022
@nnethercote
Copy link
Contributor Author

A better alternative: change args to a tagged pointer with a two bit tag. The first tag bit could represent none/some (i.e. manually doing the Option) and the second tag bit could represent infer_args.

I tried and failed with two

  • I first looked at TaggedPtr/CopyTaggedPtr, but it requires that the tagged pointer be non-null, and I need it to be possibly null for the Option functionality.
  • Then I tried having a static NONE instance of GenericArgs, with empty lists, that could be used to represent the None case. But GenericArgs contains a Span (the NONE instance would use DUMMY_SP) and Span doesn't implement Sync and therefore cannot be put into a static.

I guess I could allocate the None instance in the arena...

@petrochenkov petrochenkov 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 Sep 19, 2022
@bors
Copy link
Contributor

bors commented Sep 26, 2022

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

@nnethercote
Copy link
Contributor Author

@petrochenkov: I have addressed the comments and rebased.

@bors
Copy link
Contributor

bors commented Sep 28, 2022

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

@nnethercote
Change the "dummy self type".
5f29a13 
Because this is the only occurrence of a `Res::SelfTy` with `None` and
`None` fields, and the next commit will rely on those not being present.
@nnethercote
Shrink hir::def::Res.
f07d4ef 
`Res::SelfTy` currently has two `Option`s. When the second one is `Some`
the first one is never consulted. So we can split it into two variants,
`Res::SelfTyParam` and `Res::SelfTyAlias`, reducing the size of `Res`
from 24 bytes to 12. This then shrinks `hir::Path` and
`hir::PathSegment`, which are the HIR types that take up the most space.
@spastorino
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 29, 2022

📌 Commit f07d4ef has been approved by spastorino

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 29, 2022
@bors
Copy link
Contributor

bors commented Sep 29, 2022

⌛ Testing commit f07d4ef with merge 1bb8d27...

@bors
Copy link
Contributor

bors commented Sep 30, 2022

☀️ Test successful - checks-actions
Approved by: spastorino
Pushing 1bb8d27 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 30, 2022
@bors bors merged commit 1bb8d27 into rust-lang:master Sep 30, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 30, 2022
@nnethercote nnethercote deleted the shrink-Res branch September 30, 2022 01:38
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1bb8d27): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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
cc @rust-lang/wg-compiler-performance

Instruction count

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

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 1.2% [0.6%, 1.6%] 7
Improvements ✅
(primary)		 -0.2% [-0.3%, -0.2%] 5
Improvements ✅
(secondary)		 -0.3% [-0.4%, -0.3%] 2
All ❌✅ (primary) -0.2% [-0.3%, -0.2%] 5

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.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 3.5% [2.2%, 6.0%] 5
Improvements ✅
(primary)		 -1.6% [-3.0%, -0.4%] 14
Improvements ✅
(secondary)		 -3.5% [-6.5%, -0.9%] 9
All ❌✅ (primary) -1.6% [-3.0%, -0.4%] 14

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.

mean1 range count2
Regressions ❌
(primary)		 2.6% [2.6%, 2.6%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.9% [-2.3%, -1.5%] 2
Improvements ✅
(secondary)		 -2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) -0.4% [-2.3%, 2.6%] 3

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot added the perf-regression Performance regression. label Sep 30, 2022
@nnethercote
Copy link
Contributor Author

More keccak noise. Overall this is a small perf win, mostly on max-rss.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@spastorino spastorino

@petrochenkov petrochenkov

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
8 participants
@nnethercote @rustbot @rust-timer @bors @rust-log-analyzer @spastorino @petrochenkov @rust-highfive