Shrink `thir::Pat` #101139

nnethercote · 2022-08-28T23:49:48Z

r? @cjgillot

nnethercote · 2022-08-28T23:50:45Z

I'm not expecting big changes to perf, but let's check:

@bors try @rust-timer queue

rust-timer · 2022-08-28T23:50:46Z

Awaiting bors try build completion.

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

bors · 2022-08-28T23:50:53Z

⌛ Trying commit e61b4ab3d67e1fa18752a26fe276f84e03b307bc with merge 324c6fac56d966385f3ab26afe625faa0a58a17f...

bors · 2022-08-29T01:24:33Z

☀️ Try build successful - checks-actions
Build commit: 324c6fac56d966385f3ab26afe625faa0a58a17f (324c6fac56d966385f3ab26afe625faa0a58a17f)

rust-timer · 2022-08-29T01:24:35Z

Queued 324c6fac56d966385f3ab26afe625faa0a58a17f with parent ce36e88, future comparison URL.

rust-timer · 2022-08-29T07:48:49Z

Finished benchmarking commit (324c6fac56d966385f3ab26afe625faa0a58a17f): comparison URL.

Overall result: ❌ regressions - no 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.

@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.2%, 1.4%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.8%	[-5.8%, -5.8%]	1
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)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

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

cjgillot · 2022-08-29T17:56:15Z

Silly question: why use Box<Pat> and not a map pats: PatId -> Pat and store PatIds?

nnethercote · 2022-08-29T21:39:32Z

Silly question: why use Box<Pat> and not a map pats: PatId -> Pat and store PatIds?

The first commit log explains this:

Ideally, Pat would be stored in Thir like Expr and Stmt and
referred to with a PatId rather than Box<Pat>. But this is hard to
do because lots of Pats get created after the destruction of the Cx
that does normal THIR building. But this does get us a step closer to
PatId, because all the Box<Pat> occurrences would be replaced with
PatId if PatId ever happened.

cjgillot · 2022-08-29T22:13:58Z

Sorry, I forgot to look at the commit messages. Indeed, that was a silly question 😄
#101086 removes late creation of Pats, and may allow us to fully migrate to PatId.

@bors r+

bors · 2022-08-29T22:13:59Z

📌 Commit e61b4ab3d67e1fa18752a26fe276f84e03b307bc has been approved by cjgillot

It is now in the queue for this repository.

nnethercote · 2022-08-29T22:19:30Z

Not a silly question, a very relevant one :) Before I did this PR I tried to introduce PatId and it was a mess. I will try again after #101086 is merged.

I always try to structure my commits carefully, and I often write "best reviewed one commit at a time", but I failed to do so on this PR.

bors · 2022-09-01T19:54:22Z

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

`thir::Pat::kind` is a `Box<PatKind>`, which doesn't follow the usual pattern in AST/HIR/THIR which is that the "kind" enum for a node is stored inline within the parent struct. This commit makes the `PatKind` directly inline within the `Pat`. This requires using `Box<Pat>` in all the types that hold a `Pat. Ideally, `Pat` would be stored in `Thir` like `Expr` and `Stmt` and referred to with a `PatId` rather than `Box<Pat>`. But this is hard to do because lots of `Pat`s get created after the destruction of the `Cx` that does normal THIR building. But this does get us a step closer to `PatId`, because all the `Box<Pat>` occurrences would be replaced with `PatId` if `PatId` ever happened. At 128 bytes, `Pat` is large. Subsequent commits will shrink it.

`Builder::expr_into_pattern` has a single call site. Currently the `pattern` argument at the call site is always cloned. This commit changes things so that we instead do a clone within `expr_into_pattern`, but only if the pattern has the `PatKind::AscribeUserType` kind, and we only clone the annotation within the pattern instead of the entire pattern.

Because it's the biggest variant. Also, make `PatRange` non-`Copy`, because it's 104 bytes, which is pretty big.

This shrinks `Ascription`, which shrinks `PatKind::AscribeUserType`, which shrinks `Pat`.

To shrink it a little more.

nnethercote · 2022-09-01T21:49:05Z

I rebased to fix the conflicts.

@bors r=cjgillot

bors · 2022-09-01T21:49:07Z

📌 Commit 43a0268 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2022-09-03T04:19:03Z

⌛ Testing commit 43a0268 with merge 0421444...

bors · 2022-09-03T07:00:00Z

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

rust-timer · 2022-09-03T08:15:59Z

Finished benchmarking commit (0421444): 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
Improvements ✅ (secondary)	-0.9%	[-2.3%, -0.6%]	7
All ❌✅ (primary)	-	-	0

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)	4.2%	[4.0%, 4.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.1%	[-8.2%, -2.5%]	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)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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

nnethercote · 2022-09-19T12:03:56Z

#101086 removes late creation of Pats, and may allow us to fully migrate to PatId.

Unfortunately not, because of this sequence:

ConstToPat::recur needs Cx
- called by ConstToPat::to_pat
  - called by PatCtxt::const_to_pat
    - PatCtxt::new
      - called by MatchVisitor::lower_pattern
        - MatchVisitor created by check_match
          - called by thir::pattern:::check_match
            - which is a provider method, and thus can't be given a Cx

rust-highfive assigned cjgillot Aug 28, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 28, 2022

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

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

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

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 Aug 29, 2022

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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 1, 2022

nnethercote added 5 commits September 2, 2022 07:26

Box PatKind::Range.

2c4c8eb

Because it's the biggest variant. Also, make `PatRange` non-`Copy`, because it's 104 bytes, which is pretty big.

Box CanonicalUserTypeAnnotation::CanonicalUserType.

a40124e

This shrinks `Ascription`, which shrinks `PatKind::AscribeUserType`, which shrinks `Pat`.

Use boxed slices in PatKind.

43a0268

To shrink it a little more.

nnethercote force-pushed the shrink-thir-Pat branch from e61b4ab to 43a0268 Compare September 1, 2022 21:48

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 1, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 3, 2022

bors merged commit 0421444 into rust-lang:master Sep 3, 2022

rustbot added this to the 1.65.0 milestone Sep 3, 2022

This was referenced Sep 3, 2022

Add ConstKind::Expr #99798

Merged

Compute information about function parameters on THIR #101086

Merged

Include enum path in variant suggestion #101357

Merged

nnethercote deleted the shrink-thir-Pat branch September 4, 2022 22:41

Zalathar mentioned this pull request Jul 28, 2024

Isolate the diagnostic code that expects thir::Pat to be printable #128304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shrink `thir::Pat` #101139

Shrink `thir::Pat` #101139

nnethercote commented Aug 28, 2022

nnethercote commented Aug 28, 2022

rust-timer commented Aug 28, 2022

bors commented Aug 28, 2022

bors commented Aug 29, 2022

rust-timer commented Aug 29, 2022

rust-timer commented Aug 29, 2022

cjgillot commented Aug 29, 2022

nnethercote commented Aug 29, 2022

cjgillot commented Aug 29, 2022

bors commented Aug 29, 2022

nnethercote commented Aug 29, 2022

bors commented Sep 1, 2022

nnethercote commented Sep 1, 2022

bors commented Sep 1, 2022

bors commented Sep 3, 2022

bors commented Sep 3, 2022

rust-timer commented Sep 3, 2022

nnethercote commented Sep 19, 2022

Shrink thir::Pat #101139

Shrink thir::Pat #101139

Conversation

nnethercote commented Aug 28, 2022

nnethercote commented Aug 28, 2022

rust-timer commented Aug 28, 2022

bors commented Aug 28, 2022

bors commented Aug 29, 2022

rust-timer commented Aug 29, 2022

rust-timer commented Aug 29, 2022

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

cjgillot commented Aug 29, 2022

nnethercote commented Aug 29, 2022

cjgillot commented Aug 29, 2022

bors commented Aug 29, 2022

nnethercote commented Aug 29, 2022

bors commented Sep 1, 2022

nnethercote commented Sep 1, 2022

bors commented Sep 1, 2022

bors commented Sep 3, 2022

bors commented Sep 3, 2022

rust-timer commented Sep 3, 2022

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

nnethercote commented Sep 19, 2022

Shrink `thir::Pat` #101139

Shrink `thir::Pat` #101139