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

Shrink thir::Pat #101139

Merged
merged 5 commits into from
Sep 3, 2022
Merged

Shrink thir::Pat #101139

merged 5 commits into from
Sep 3, 2022

Conversation

nnethercote
Copy link
Contributor

@rustbot 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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2022
@nnethercote
Copy link
Contributor Author

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

@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 Aug 28, 2022
@bors
Copy link
Contributor

bors commented Aug 28, 2022

⌛ Trying commit e61b4ab3d67e1fa18752a26fe276f84e03b307bc with merge 324c6fac56d966385f3ab26afe625faa0a58a17f...

@bors
Copy link
Contributor

bors commented Aug 29, 2022

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

@rust-timer
Copy link
Collaborator

Queued 324c6fac56d966385f3ab26afe625faa0a58a17f with parent ce36e88, future comparison URL.

@rust-timer
Copy link
Collaborator

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.

mean1 range count2
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.

mean1 range count2
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.

mean1 range count2
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

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

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

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

@nnethercote
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

bors commented Aug 29, 2022

📌 Commit e61b4ab3d67e1fa18752a26fe276f84e03b307bc has been approved by cjgillot

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 Aug 29, 2022
@nnethercote
Copy link
Contributor Author

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
Copy link
Contributor

bors commented Sep 1, 2022

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

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

I rebased to fix the conflicts.

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Sep 1, 2022

📌 Commit 43a0268 has been approved by cjgillot

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 1, 2022
@bors
Copy link
Contributor

bors commented Sep 3, 2022

⌛ Testing commit 43a0268 with merge 0421444...

@bors
Copy link
Contributor

bors commented Sep 3, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 3, 2022
@bors bors merged commit 0421444 into rust-lang:master Sep 3, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 3, 2022
@rust-timer
Copy link
Collaborator

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.

mean1 range count2
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.

mean1 range count2
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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@nnethercote nnethercote deleted the shrink-thir-Pat branch September 4, 2022 22:41
@nnethercote
Copy link
Contributor Author

#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

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

Successfully merging this pull request may close these issues.

6 participants