Stop considering moved-out locals when computing auto traits for generators #112279

nbdd0121 · 2023-06-04T16:01:08Z

Addresses #94067 (but does not fix it since drop-tracking-mir is unstable)

This PR, unlike #110420 or #112050, does not attempt to reduce the number of live locals across suspension points. It however ignores moved-out locals for trait purposes. So this PR solves the non-send issue, but not the size issue.

Suggested by @RalfJung in rust-lang/unsafe-code-guidelines#188

cc @b-naber who's working on this from a different perspective.

r? @cjgillot

rustbot · 2023-06-04T16:01:15Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

cjgillot · 2023-06-11T13:07:36Z

compiler/rustc_mir_transform/src/generator.rs

+            if !init_locals.contains(local) {
+                // If only the storage is required to be live, but local is not initialized, then we can
+                // ignore such type for auto trait purposes.
+                true


Why do we require storage to be live if the locals is not initialized? Should the general algorithm be changed to avoid requiring storage? That'd kill 2 birds with one stone: the spurious auto trait limitation and the generator size.

As a general rule, I'd rather keep the "storage" and the "traits" algorithm as close to each other as possible: any deviation between the two is a potential source of unsoundness. This exact change is probably not, but the general rule should stand.

That's currently an open question rust-lang/unsafe-code-guidelines#188 and is much harder to solve.

cjgillot · 2023-07-08T15:43:35Z

There is a meeting on july 24th to discuss soundness of -Zdrop-tracking-mir. I'm waiting for this meeting before doing any review here.

bors · 2023-08-09T18:43:25Z

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

nbdd0121 · 2023-08-18T15:44:12Z

What's the outcome from the meeting?

cjgillot · 2023-08-24T16:30:52Z

The meeting happened and decided to stabilize -Zdrop-tracking-mir.

On the change itself: I'd still prefer to be prudent about any divergence between the "trait" algorithm and the "storage" algorithm.
I understand that this PR emulates drop elaboration for the "trait" algorithm: if a local is moved-from on all paths that lead to a drop, then it does not need dropping.
Moving drop elaboration earlier has long been in the borrowck wishlist, but is hard enough to stay there long.

My proposal:

document that this attempts to emulate drop elaboration, a kind of implicit RemoveUninitDrops pass;
wait after Enable -Zdrop-tracking-mir by default #107421 merges;
run perf then to check we don't have a regression (MaybeInitializedLocals can be quite slow, as it tracks places, not just locals).

bors · 2023-09-23T20:49:54Z

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

These tests are identical to the ones without drop-track prefix, so remove.

…rators

cjgillot · 2023-10-04T21:14:38Z

I said I would run perf, and forgot.
@bors try @rust-timer queue

bors · 2023-10-04T21:15:49Z

⌛ Trying commit 0259c6e with merge 258decf...

Stop considering moved-out locals when computing auto traits for generators Addresses rust-lang#94067 (but does not fix it since drop-tracking-mir is unstable) This PR, unlike rust-lang#110420 or rust-lang#112050, does not attempt to reduce the number of live locals across suspension points. It however ignores moved-out locals for trait purposes. So this PR solves the non-send issue, but not the size issue. Suggested by `@RalfJung` in [rust-lang/unsafe-code-guidelines#188](rust-lang/unsafe-code-guidelines#188 (comment)) cc `@b-naber` who's working on this from a different perspective. r? `@cjgillot`

cjgillot · 2023-10-04T21:18:32Z

One limitation of this PR is the interaction with drop-elaboration.
@RalfJung cited in #107421 (comment) an earlier adventure between drop elaboration and const checking.

I still have my concern about the auto-trait analysis and the layout analysis eventually drifting.

bors · 2023-10-04T22:29:52Z

☀️ Try build successful - checks-actions
Build commit: 258decf (258decfeb5bf65897ee095291294f5800ad13f51)

rust-timer · 2023-10-05T00:15:27Z

Finished benchmarking commit (258decf): comparison URL.

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

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -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.6%	[0.4%, 0.8%]	5
Regressions ❌ (secondary)	3.0%	[0.6%, 6.1%]	15
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.4%, 0.8%]	5

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

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)	3.7%	[2.8%, 6.4%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 623.925s -> 621.686s (-0.36%)
Artifact size: 272.03 MiB -> 272.00 MiB (-0.01%)

RalfJung · 2023-10-05T05:13:48Z

One limitation of this PR is the interaction with drop-elaboration.

So this passes uses post-drop-elaboration MIR? Yeah that would be concerning.
Can it use pre-drop-elaboration MIR instead?

cjgillot · 2023-10-05T10:51:54Z

One limitation of this PR is the interaction with drop-elaboration.

So this passes uses post-drop-elaboration MIR? Yeah that would be concerning. Can it use pre-drop-elaboration MIR instead?

The current pass uses pre-drop-elaboration MIR. This PR proposes to elaborate some drops as part of the analysis.

I'm not fully aware of the difficulties around drop elaboration, so we need a bit of background on the foreseeable difficulties.

apiraino · 2023-11-21T15:35:26Z

I think this might waiting on triaging the latest perf run

@rustbot author

Dylan-DPC · 2024-02-10T05:43:35Z

@nbdd0121 any updates on this?

rustbot assigned cjgillot Jun 4, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 4, 2023

cjgillot reviewed Jun 11, 2023

View reviewed changes

nbdd0121 added 2 commits September 24, 2023 04:58

Remove drop-track-field-assign[-nonsend] test

425e87e

These tests are identical to the ones without drop-track prefix, so remove.

Stop considering moved-out locals when computing auto traits for gene…

0259c6e

…rators

nbdd0121 force-pushed the generator branch from 9652ea9 to 0259c6e Compare September 24, 2023 07:13

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 4, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 5, 2023

This comment was marked as duplicate.

Sign in to view

rustbot 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 Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop considering moved-out locals when computing auto traits for generators #112279

Stop considering moved-out locals when computing auto traits for generators #112279

nbdd0121 commented Jun 4, 2023 •

edited

Loading

rustbot commented Jun 4, 2023

cjgillot Jun 11, 2023

nbdd0121 Jun 11, 2023

cjgillot commented Jul 8, 2023

bors commented Aug 9, 2023

nbdd0121 commented Aug 18, 2023

cjgillot commented Aug 24, 2023 •

edited

Loading

bors commented Sep 23, 2023

cjgillot commented Oct 4, 2023

This comment has been minimized.

bors commented Oct 4, 2023

cjgillot commented Oct 4, 2023 •

edited

Loading

bors commented Oct 4, 2023

This comment has been minimized.

rust-timer commented Oct 5, 2023

RalfJung commented Oct 5, 2023

cjgillot commented Oct 5, 2023

This comment was marked as duplicate.

apiraino commented Nov 21, 2023

Dylan-DPC commented Feb 10, 2024

Stop considering moved-out locals when computing auto traits for generators #112279

Are you sure you want to change the base?

Stop considering moved-out locals when computing auto traits for generators #112279

Conversation

nbdd0121 commented Jun 4, 2023 • edited Loading

rustbot commented Jun 4, 2023

cjgillot Jun 11, 2023

Choose a reason for hiding this comment

nbdd0121 Jun 11, 2023

Choose a reason for hiding this comment

cjgillot commented Jul 8, 2023

bors commented Aug 9, 2023

nbdd0121 commented Aug 18, 2023

cjgillot commented Aug 24, 2023 • edited Loading

bors commented Sep 23, 2023

cjgillot commented Oct 4, 2023

This comment has been minimized.

bors commented Oct 4, 2023

cjgillot commented Oct 4, 2023 • edited Loading

bors commented Oct 4, 2023

This comment has been minimized.

rust-timer commented Oct 5, 2023

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

RalfJung commented Oct 5, 2023

cjgillot commented Oct 5, 2023

This comment was marked as duplicate.

apiraino commented Nov 21, 2023

Dylan-DPC commented Feb 10, 2024

nbdd0121 commented Jun 4, 2023 •

edited

Loading

cjgillot commented Aug 24, 2023 •

edited

Loading

cjgillot commented Oct 4, 2023 •

edited

Loading