dropck_outlives check whether generator witness needs_drop #117134

lcnr · 2023-10-24T17:19:25Z

see https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/.23116242.3A.20Code.20no.20longer.20compiles.20after.20-Zdrop-tracking-mir.20.E2.80.A6/near/398311627 for an explanation.

Fixes #116242 (or well, the repro by @jamuraa in #116242 (comment)). I did not add a regression test as it depends on other crates. We do have 1 test going from fail to pass, showing the intended behavior.

r? types

lcnr · 2023-10-24T17:26:30Z

@bors try

bors · 2023-10-24T17:27:39Z

⌛ Trying commit 2ca4c7e with merge 2cf1c2a...

dropck_outlives check whether generator witness needs_drop see https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/.23116242.3A.20Code.20no.20longer.20compiles.20after.20-Zdrop-tracking-mir.20.E2.80.A6/near/398311627 for an explanation. Fixes rust-lang#116242 (or well, the repro by `@jamuraa` in rust-lang#116242 (comment)). I did not add a regression test as it depends on other crates. We do have 1 test going from fail to pass, showing the intended behavior. r? types

lcnr · 2023-10-24T17:27:55Z

compiler/rustc_ty_utils/src/needs_drop.rs

+                    // need to be dropped, and only require the captured types to be live
+                    // if they do.
+                    ty::Coroutine(_, args, _) => {
+                        if self.reveal_coroutine_witnesses {


this is necessary to handle nested futures 🤔 should potentially add a test for it, but I don't feel motivated enough to do so 😅

lcnr · 2023-10-24T17:29:13Z

want to run both crater and perf. Worried that I messed up and we get some weird cycles because of this, I am fairly confident that this is alright.

bors · 2023-10-24T18:49:54Z

☀️ Try build successful - checks-actions
Build commit: 2cf1c2a (2cf1c2a614d6ae3bd50a8df20aeac2f9eceeea87)

cjgillot · 2023-10-25T17:51:00Z

Thanks for figuring this out @lcnr!

lcnr · 2023-10-26T07:38:02Z

@rust-timer queue

@craterbot check

craterbot · 2023-10-26T07:38:08Z

👌 Experiment pr-117134 created and queued.
🤖 Automatically detected try build 2cf1c2a
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

lcnr · 2023-10-26T07:49:38Z

assuming that the crater run and perf is acceptable, and to make sure we backport in time:

To quickly summarize again: the change from this PR is in bold

dropping a coroutine witness drops the values stored in its state machine: the content of its CoroutineWitness
computing the CoroutineWitness relies on mir_built
mir_built relies on fn needs_drop, we conservatively assume that all generators have to be dropped
we do not know the exact lifetimes of the values stored in the witness (as they may be self-referential), so for a coroutine to be drop live during mir_borrowck we require anything that could theoretically be accessed by the coroutine to be live.
however, during mir_borrowck we can access the CoroutineWitness, so we only require everything to be live if the coroutine actually contains any field which requires drop. This recursively looks into nested coroutines.

I believe that this does not result in query cycles as the only way needs_drop can access nested coroutines if they are from the same body. Coroutines from other bodies are hidden by an opaque type, which we should not be able to reveal.

@rfcbot fcp merge

rfcbot · 2023-10-26T07:49:39Z

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

lcnr · 2023-10-26T07:50:44Z

@craterbot p=1

craterbot · 2023-10-26T07:50:46Z

📝 Configuration of the pr-117134 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

lqd · 2023-10-26T08:12:28Z

@rust-timer build 2cf1c2a

lcnr · 2023-11-02T20:29:55Z

@bors r=compiler-errors

bors · 2023-11-02T20:29:58Z

📌 Commit dda5e32 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-11-02T22:03:41Z

⌛ Testing commit dda5e32 with merge a2f5f96...

bors · 2023-11-03T00:01:37Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing a2f5f96 to master...

rust-timer · 2023-11-03T02:09:13Z

Finished benchmarking commit (a2f5f96): comparison URL.

Overall result: ❌ regressions - 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.3%	[0.2%, 0.3%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.3%]	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)	5.8%	[5.8%, 5.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-2.9%, -2.9%]	1
All ❌✅ (primary)	5.8%	[5.8%, 5.8%]	1

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

Binary size

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

Bootstrap: 636.864s -> 635.107s (-0.28%)
Artifact size: 304.51 MiB -> 304.48 MiB (-0.01%)

82: Automated pull from upstream `master` r=tshepang a=github-actions[bot] This PR pulls the following changes from the upstream repository: * rust-lang/rust#117313 * rust-lang/rust#117131 * rust-lang/rust#117134 * rust-lang/rust#117471 * rust-lang/rust#117521 * rust-lang/rust#117513 * rust-lang/rust#117512 * rust-lang/rust#117509 * rust-lang/rust#117495 * rust-lang/rust#117394 * rust-lang/rust#117466 * rust-lang/rust#117204 * rust-lang/rust#117386 * rust-lang/rust#117506 Co-authored-by: Nicholas Nethercote <[email protected]> Co-authored-by: roblabla <[email protected]> Co-authored-by: Michael Goulet <[email protected]> Co-authored-by: massivebird <[email protected]> Co-authored-by: bors <[email protected]> Co-authored-by: Zalathar <[email protected]> Co-authored-by: lcnr <[email protected]> Co-authored-by: Joshua Liebow-Feeser <[email protected]> Co-authored-by: Matthias Krüger <[email protected]>

pnkfelix · 2023-11-07T16:14:53Z

regressed ripgrep check for incr-patched:println (0.35%), incr-full (0.26%), and full (0.21%); regressed regex check for incr-patched:Job (0.29%) and incr-patched:println (0.24%).
this is easily justified; it is fixing a code-acceptance regression
marking as triaged

@rustbot label: +perf-regression-triaged

wesleywiser · 2023-11-08T16:11:05Z

Accepting for beta backport as discussed during a previous compiler team triage meeting.

[beta] backports - dropck_outlives check whether generator witness needs_drop rust-lang#117134 - Make sure that predicates with unmentioned bound vars are still considered global in the old solver rust-lang#117589 - Check binders with bound vars for global bounds that don't hold rust-lang#117637 - generator layout: ignore fake borrows rust-lang#117712 r? ghost

rustbot assigned compiler-errors Oct 24, 2023

lcnr added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 24, 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 Oct 24, 2023

lcnr force-pushed the dropck_outlives-coroutine branch from cb8123f to 063cca2 Compare October 24, 2023 17:24

lcnr added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Oct 24, 2023

lcnr force-pushed the dropck_outlives-coroutine branch from 063cca2 to 2ca4c7e Compare October 24, 2023 17:26

lcnr commented Oct 24, 2023

View reviewed changes

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 26, 2023

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 26, 2023

lcnr removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 26, 2023

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 26, 2023

This comment has been minimized.

Sign in to view

review + add tests

dda5e32

lcnr force-pushed the dropck_outlives-coroutine branch from 41a674e to dda5e32 Compare November 2, 2023 17:16

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 Nov 2, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 3, 2023

bors merged commit a2f5f96 into rust-lang:master Nov 3, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Nov 3, 2023

github-actions bot mentioned this pull request Nov 3, 2023

Automated pull from upstream master ferrocene/ferrocene#82

Merged

lcnr added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 3, 2023

lcnr deleted the dropck_outlives-coroutine branch November 3, 2023 09:28

rustbot added the perf-regression-triaged The performance regression has been triaged. label Nov 7, 2023

wesleywiser added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 8, 2023

cuviper mentioned this pull request Nov 9, 2023

[beta] backports #117764

Merged

cuviper modified the milestones: 1.75.0, 1.74.0 Nov 9, 2023

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 9, 2023

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 9, 2023

apiraino removed the to-announce Announce this issue on triage meeting label Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dropck_outlives check whether generator witness needs_drop #117134

dropck_outlives check whether generator witness needs_drop #117134

lcnr commented Oct 24, 2023

lcnr commented Oct 24, 2023

bors commented Oct 24, 2023

lcnr Oct 24, 2023

lcnr commented Oct 24, 2023

bors commented Oct 24, 2023

cjgillot commented Oct 25, 2023

lcnr commented Oct 26, 2023

This comment has been minimized.

craterbot commented Oct 26, 2023

lcnr commented Oct 26, 2023 •

edited

Loading

rfcbot commented Oct 26, 2023 •

edited by spastorino

Loading

lcnr commented Oct 26, 2023

craterbot commented Oct 26, 2023

lqd commented Oct 26, 2023

This comment has been minimized.

lcnr commented Nov 2, 2023

bors commented Nov 2, 2023

bors commented Nov 2, 2023

bors commented Nov 3, 2023

rust-timer commented Nov 3, 2023

pnkfelix commented Nov 7, 2023

wesleywiser commented Nov 8, 2023

dropck_outlives check whether generator witness needs_drop #117134

dropck_outlives check whether generator witness needs_drop #117134

Conversation

lcnr commented Oct 24, 2023

lcnr commented Oct 24, 2023

bors commented Oct 24, 2023

lcnr Oct 24, 2023

Choose a reason for hiding this comment

lcnr commented Oct 24, 2023

bors commented Oct 24, 2023

cjgillot commented Oct 25, 2023

lcnr commented Oct 26, 2023

This comment has been minimized.

craterbot commented Oct 26, 2023

lcnr commented Oct 26, 2023 • edited Loading

rfcbot commented Oct 26, 2023 • edited by spastorino Loading

lcnr commented Oct 26, 2023

craterbot commented Oct 26, 2023

lqd commented Oct 26, 2023

This comment has been minimized.

lcnr commented Nov 2, 2023

bors commented Nov 2, 2023

bors commented Nov 2, 2023

bors commented Nov 3, 2023

rust-timer commented Nov 3, 2023

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

pnkfelix commented Nov 7, 2023

wesleywiser commented Nov 8, 2023

lcnr commented Oct 26, 2023 •

edited

Loading

rfcbot commented Oct 26, 2023 •

edited by spastorino

Loading