Shrink `hir::def::Res` by nnethercote · Pull Request #101887 · rust-lang/rust

nnethercote · 2022-09-16T07:40:15Z

r? @spastorino

rustbot · 2022-09-16T07:40:19Z

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

nnethercote · 2022-09-16T07:40:25Z

@bors try @rust-timer queue

rust-timer · 2022-09-16T07:40:26Z

Awaiting bors try build completion.

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

bors · 2022-09-16T07:40:35Z

⌛ Trying commit a8492962c36d63271af04d85fb20e811b045bce2 with merge 2e42f14cf381b553c44ffca6fba169be755ea436...

nnethercote · 2022-09-16T10:25:50Z

@bors try @rust-timer queue

rust-timer · 2022-09-16T10:25:52Z

Awaiting bors try build completion.

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

bors · 2022-09-16T10:26:00Z

⌛ Trying commit 4ca8b9fff9022c84a4bee41cacbdc20ceab35dbc with merge cc30719e15b97b59204b625423adb870a39b07ce...

compiler/rustc_hir/src/def.rs

nnethercote · 2022-09-16T12:38:15Z

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 · 2022-09-16T23:15:54Z

@bors try @rust-timer queue

rust-timer · 2022-09-16T23:15:55Z

Awaiting bors try build completion.

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

bors · 2022-09-16T23:16:04Z

⌛ Trying commit d48b6a9c6c0023d3a4a6fc6db6f2e2dc17d04deb with merge a276ba6fd94b4c292920ebd9bede877948cb9169...

nnethercote · 2022-09-19T07:13:28Z

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

bors · 2022-09-26T17:07:38Z

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

nnethercote · 2022-09-28T06:28:32Z

@petrochenkov: I have addressed the comments and rebased.

bors · 2022-09-28T18:54:48Z

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

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.

`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 · 2022-09-29T19:58:18Z

@bors r+

bors · 2022-09-29T19:58:20Z

📌 Commit f07d4ef has been approved by spastorino

It is now in the queue for this repository.

bors · 2022-09-29T22:45:28Z

⌛ Testing commit f07d4ef with merge 1bb8d27...

bors · 2022-09-30T01:32:09Z

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

rust-timer · 2022-09-30T03:57:46Z

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.

	mean¹	range	count²
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.

	mean¹	range	count²
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.

	mean¹	range	count²
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

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-09-30T07:48:08Z

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

@rustbot label: +perf-regression-triaged

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

rust-highfive assigned spastorino Sep 16, 2022

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

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

petrochenkov self-assigned this Sep 16, 2022

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

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 force-pushed the shrink-Res branch from a849296 to 4ca8b9f Compare September 16, 2022 10:25

This comment was marked as resolved.

Sign in to view

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 Outdated Show resolved Hide resolved

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

This comment has been minimized.

Sign in to view

nnethercote force-pushed the shrink-Res branch from 4ca8b9f to d48b6a9 Compare September 16, 2022 23:14

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 17, 2022

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2022

nnethercote force-pushed the shrink-Res branch from 9c44a36 to 8948feb Compare September 19, 2022 06:15

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

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

nnethercote force-pushed the shrink-Res branch from 8948feb to a78f47a Compare September 28, 2022 06:25

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 force-pushed the shrink-Res branch from a78f47a to 92b8cfd Compare September 28, 2022 21:14

nnethercote force-pushed the shrink-Res branch from 92b8cfd to f07d4ef Compare September 28, 2022 22:45

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 added the merged-by-bors This PR was explicitly merged by bors. label Sep 30, 2022

bors merged commit 1bb8d27 into rust-lang:master Sep 30, 2022

rustbot added this to the 1.66.0 milestone Sep 30, 2022

nnethercote deleted the shrink-Res branch September 30, 2022 01:38

rustbot added the perf-regression Performance regression. label Sep 30, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 30, 2022

Uh oh!

Conversation

nnethercote commented Sep 16, 2022

Uh oh!

rustbot commented Sep 16, 2022

Uh oh!

nnethercote commented Sep 16, 2022

Uh oh!

rust-timer commented Sep 16, 2022

Uh oh!

bors commented Sep 16, 2022

Uh oh!

This comment has been minimized.

This comment was marked as resolved.

nnethercote commented Sep 16, 2022

Uh oh!

rust-timer commented Sep 16, 2022

Uh oh!

bors commented Sep 16, 2022

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

nnethercote commented Sep 16, 2022

Uh oh!

nnethercote commented Sep 16, 2022

Uh oh!

rust-timer commented Sep 16, 2022

Uh oh!

bors commented Sep 16, 2022

Uh oh!

nnethercote commented Sep 19, 2022

Uh oh!

bors commented Sep 26, 2022

Uh oh!

nnethercote commented Sep 28, 2022

Uh oh!

bors commented Sep 28, 2022

Uh oh!

spastorino commented Sep 29, 2022

Uh oh!

bors commented Sep 29, 2022

Uh oh!

bors commented Sep 29, 2022

Uh oh!

bors commented Sep 30, 2022

Uh oh!

rust-timer commented Sep 30, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

nnethercote commented Sep 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants