Skip to content

give noalias back to refs in closures - #162361

Open
RalfJung wants to merge 3 commits into
rust-lang:mainfrom
RalfJung:maybe-dangling-perf2
Open

RalfJung wants to merge 3 commits into
rust-lang:mainfrom
RalfJung:maybe-dangling-perf2

Conversation

@RalfJung

@RalfJung RalfJung commented Sep 6, 2026

Copy link
Copy Markdown
Member

View all comments

Reverts parts of #160745 to fix #163220.

Needs team approval; FCP is happening in #163220.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 6, 2026
@RalfJung

RalfJung commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

@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 Sep 6, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Sep 6, 2026
perf experiment: give noalias back to refs in closures
@rust-log-analyzer

This comment has been minimized.

@rust-bors

rust-bors Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 53adad8 (53adad86ea8dd1518c719893cfc8f19dc743b965)
Base parent: 46a266b (46a266b9fb0fb75e05fa4c9132f7355fcb62d4a4)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (53adad8): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.4%] 4
Improvements ✅
(primary)
-0.3% [-0.7%, -0.2%] 67
Improvements ✅
(secondary)
-0.3% [-0.4%, -0.2%] 19
All ❌✅ (primary) -0.3% [-0.7%, -0.2%] 67

Max RSS (memory usage)

Results (primary -4.5%, secondary 1.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.5% [2.5%, 4.5%] 2
Improvements ✅
(primary)
-4.5% [-4.5%, -4.5%] 1
Improvements ✅
(secondary)
-3.5% [-3.5%, -3.5%] 1
All ❌✅ (primary) -4.5% [-4.5%, -4.5%] 1

Cycles

Results (primary 0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
3.2% [3.2%, 3.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.4% [-2.4%, -2.4%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [-2.4%, 3.2%] 2

Binary size

Results (primary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Bootstrap: 478.283s -> 476.002s (-0.48%)
Artifact size: 403.38 MiB -> 403.31 MiB (-0.02%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 6, 2026
@shepmaster

shepmaster commented Sep 23, 2026

Copy link
Copy Markdown
Member

I have a criterion benchmark of some AVX2 hashing code on x86_64 Windows which reports...

commit speed (GiB/s)
aa05936 (1.99.0-beta.7) 57.537
1a82b5d (main) 23.559
1a82b5d (main) + reverting the 4 commits from #160745 58.155
1a82b5d (main) + cherry-picking this commit 59.121

I am 👍 for the performance results from this PR 😉

(Investigation occurred on Zulip)

@RalfJung

Copy link
Copy Markdown
Member Author

Is this a small piece of code you can share?

Either that code has a FnOnce closure that captures exactly 1 or 2 references and the noalias they get is somehow very important, or there's something going on we do not understand.

@RalfJung
RalfJung force-pushed the maybe-dangling-perf2 branch from d5af325 to 2d88470 Compare September 23, 2026 16:10
@RalfJung
RalfJung marked this pull request as ready for review September 23, 2026 16:21
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2026
@rustbot

rustbot commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 23, 2026
@rustbot

rustbot commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

r? @Darksonn

rustbot has assigned @Darksonn.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @ChrisDenton, libs
  • @ChrisDenton, libs expanded to 13 candidates
  • Random selection from 7 candidates

@RalfJung

Copy link
Copy Markdown
Member Author

Let's make sure this still fixes the regression (the previous run didn't have the MaybeDangling for coroutines but we need that).
@bors try
@rust-timer queue

r? @saethlin

@rust-timer

This comment has been minimized.

@rustbot rustbot assigned saethlin and unassigned Darksonn Sep 23, 2026
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 23, 2026
@rust-bors

This comment has been minimized.

@RalfJung
RalfJung force-pushed the maybe-dangling-perf2 branch from cfdb944 to b0e171e Compare September 23, 2026 16:23
@RalfJung RalfJung changed the title perf experiment: give noalias back to refs in closures give noalias back to refs in closures Sep 23, 2026
@RalfJung

Copy link
Copy Markdown
Member Author

@bors try

rust-bors Bot pushed a commit that referenced this pull request Sep 23, 2026
give noalias back to refs in closures
@rust-bors

This comment has been minimized.

|| def.flags().contains(AdtFlags::IS_MANUALLY_DROP)
}
ty::Closure(..) | ty::Coroutine(..) | ty::CoroutineClosure(..) => true,
ty::Coroutine(..) | ty::CoroutineClosure(..) => true,

@RalfJung RalfJung Sep 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure what a CoroutineClosure is / why it is a separate type. But I'll assume it contains a corotuine so we should have MaybeDangling there.

View changes since the review

@rust-log-analyzer

This comment has been minimized.

@RalfJung
RalfJung force-pushed the maybe-dangling-perf2 branch from b0e171e to a18393e Compare September 23, 2026 17:02
@rust-bors

rust-bors Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 47167be (47167bec2164397e62586212b9a876a49c2f8df7)
Base parent: 9a9d194 (9a9d1946d1e119676b7a6a8a26a264061e61027d)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (47167be): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 4
Improvements ✅
(primary)
-0.3% [-0.7%, -0.2%] 83
Improvements ✅
(secondary)
-0.3% [-0.5%, -0.2%] 19
All ❌✅ (primary) -0.3% [-0.7%, -0.2%] 83

Max RSS (memory usage)

Results (primary -5.6%, secondary 0.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Cycles

Results (secondary -11.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.9% [2.6%, 5.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-16.3% [-32.3%, -3.3%] 7
All ❌✅ (primary) - - 0

Binary size

Results (primary 0.0%, secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Bootstrap: 489.957s -> 489.572s (-0.08%)
Artifact size: 407.04 MiB -> 406.27 MiB (-0.19%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 23, 2026
@saethlin

Copy link
Copy Markdown
Member

@bors r+

@rust-bors

rust-bors Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

📌 Commit a18393e has been approved by saethlin

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 Sep 23, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Sep 23, 2026
give noalias back to refs in closures



Reverts parts of #160745 to fix #163220.

Needs team approval; FCP is happening in #163220.
@rust-bors rust-bors Bot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 23, 2026
@rust-bors

rust-bors Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

💔 Test for 7f1e616 failed: CI. Failed job:

@saethlin

Copy link
Copy Markdown
Member

GHA raw logs seem to be missing:

<Error>
<Code>BlobNotFound</Code>
<Message>
The specified blob does not exist. RequestId:73546f72-f01e-0013-25b9-4bf0fc000000 Time:2026-09-24T00:15:15.1139570Z
</Message>
</Error>

And in the web UI the logs look truncated. So a GitHub bug?

@bors retry

@rust-bors rust-bors Bot 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 Sep 24, 2026
@jhpratt

jhpratt commented Sep 24, 2026

Copy link
Copy Markdown
Member

@bors try jobs=test-x86_64-gnu-distcheck

@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Sep 24, 2026
give noalias back to refs in closures


try-job: test-x86_64-gnu-distcheck
@rust-bors

rust-bors Bot commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 4c92bf3 (4c92bf3119aad084e99e6e842500386e346dbab4)
Base parent: 6eeff9a (6eeff9a52c3e35c4c4cbf5651f342dcd2191866f)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regression Performance regression. 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.

Performance regression in Criterion benchmark of AVX2 code on x86_64-pc-windows-msvc

9 participants