aligns the behavior to that prior to #118311 #118411

bvanjoi · 2023-11-28T15:08:31Z

After #118311, it seems that due to an oversight some alignments were unintentionally omitted, possibly leading the code into different branches. This PR attempts to restore those alignments and aims to fix the regression reported at #118319 (comment)

rustbot · 2023-11-28T15:08:41Z

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

compiler-errors · 2023-11-28T15:57:24Z

@bors try @rust-timer queue

bors · 2023-11-28T15:58:35Z

⌛ Trying commit 4307541 with merge 3c773cd...

…e_2, r=<try> aligns the behavior to that prior to rust-lang#118311 After rust-lang#118311, it seems that due to an oversight some alignments were unintentionally omitted, possibly leading the code into different branches. This PR attempts to restore those alignments and aims to fix the regression reported at rust-lang#118319 (comment)

bors · 2023-11-28T17:24:42Z

☀️ Try build successful - checks-actions
Build commit: 3c773cd (3c773cd98dc6c897d38d307de1cd50c92d2ad74e)

rust-timer · 2023-11-28T22:03:23Z

Finished benchmarking commit (3c773cd): comparison URL.

Overall result: no relevant changes - 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-perf -perf-regression

Instruction count

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

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

Cycles

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

Binary size

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

Bootstrap: 672.731s -> 673.339s (0.09%)
Artifact size: 313.33 MiB -> 313.36 MiB (0.01%)

rustbot · 2023-11-29T16:05:14Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

bvanjoi · 2023-11-29T16:05:41Z

I can confirm that it's not equivalent between ExprKind::Closure(Closure { movability: Some(_), .. }) => DefKind::Coroutine and tcx.coroutine_kind(def_id).is_some() for the same def_id.(Additionally, I'm curious as to why the test suite passed without issues)

So, the latest patch modifies tcx.is_coroutine to be more similar to the def_kind prior to #118188, with the hope that it will have a positive impact on performance.

bvanjoi · 2023-11-29T17:07:11Z

hi @compiler-errors could you help rerun this perf test?

compiler-errors · 2023-12-05T16:04:27Z

@bors try @rust-timer queue

bors · 2023-12-05T16:05:40Z

⌛ Trying commit 77f9a77 with merge 205cae8...

…e_2, r=<try> aligns the behavior to that prior to rust-lang#118311 After rust-lang#118311, it seems that due to an oversight some alignments were unintentionally omitted, possibly leading the code into different branches. This PR attempts to restore those alignments and aims to fix the regression reported at rust-lang#118319 (comment)

bors · 2023-12-05T17:31:22Z

☀️ Try build successful - checks-actions
Build commit: 205cae8 (205cae860ab28f4327b8e9127118187c4a411c3d)

rust-timer · 2023-12-05T20:02:59Z

Finished benchmarking commit (205cae8): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	4.4%	[0.3%, 14.2%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.7%, -0.1%]	21
Improvements ✅ (secondary)	-0.5%	[-0.8%, -0.2%]	7
All ❌✅ (primary)	0.7%	[-0.7%, 14.2%]	27

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)	-4.7%	[-7.5%, -1.7%]	4
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	-2.6%	[-7.5%, 5.8%]	5

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)	5.3%	[2.0%, 13.4%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-0.8%, -0.5%]	2
All ❌✅ (primary)	5.3%	[2.0%, 13.4%]	5

Binary size

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

Bootstrap: 675.58s -> 673.858s (-0.25%)
Artifact size: 314.11 MiB -> 314.16 MiB (0.02%)

apiraino · 2023-12-28T11:49:22Z

IIUC this is waiting on a decision in #118436, so I'll earmark this PR as waiting on author to mark this to unblock it. thanks

@rustbot author

petrochenkov · 2023-12-28T16:11:41Z

r? @compiler-errors as the author of #119174 #119258 #119198

compiler-errors · 2023-12-28T16:13:19Z

@bvanjoi:

I can confirm that it's not equivalent between ExprKind::Closure(Closure { movability: Some(_), .. }) => DefKind::Coroutine and tcx.coroutine_kind(def_id).is_some() for the same def_id.(Additionally, I'm curious as to why the test suite passed without issues)

In what case is this not true?

compiler-errors · 2023-12-28T16:23:24Z

In any case, it's certainly true now, and any coroutines that had movability: None (or worse, closures that had movability: Some(_)) were being treated incorrectly by the compiler previously.

compiler-errors · 2023-12-28T16:25:44Z

compiler/rustc_mir_build/src/build/mod.rs

@@ -474,7 +474,9 @@ fn construct_fn<'tcx>(
    };

    let mut abi = fn_sig.abi;
-    if let DefKind::Closure = tcx.def_kind(fn_def) {
+    if let DefKind::Closure = tcx.def_kind(fn_def)
+        && !tcx.is_coroutine(fn_def)


This concerns me, since I hope that coroutines don't have abi::RustCall.

Previously, DefKind included both DefKind::Closure and DefKind::Coroutine. This change is simply to ensure that they are treated the same.

bvanjoi · 2023-12-28T16:59:56Z

In what case is this not true?

I forgot about this case and I will investigate it again tomorrow

compiler-errors · 2023-12-28T17:11:13Z

Also, TyCtxt::is_closure probably has some call sites that are wrong -- could you check those too, if you haven't already?

bvanjoi · 2024-01-02T14:52:05Z

A considerable amount of time has passed, so I need to go over it again:

Firstly, I submitted merge DefKind::Coroutine into Defkind::Closure #118311, which postponed the differentiation between DefKind::Closure and DefKind::Coroutine. Subsequently, I utilized tcx.coroutine_kind to set them apart.
However, these changes led to a performance regression. Therefore, I sought to identify the differences before and after implementing merge DefKind::Coroutine into Defkind::Closure #118311. Ultimately, I noticed some discrepancies between the usage of movability and tcx.coroutine_kind. These differences can be seen at aligns the behavior to that prior to #118311 #118411 (comment)
Nonetheless, several alterations have occurred, and I think this change has already carried out the modifications I intended to introduce in this PR.
Now, I suspect that the latest commit might introduce some changes to the benchmark.

I can confirm that it's not equivalent....

I'm currently unable to reproduce it..🤦

bvanjoi · 2024-01-02T15:06:01Z

@compiler-errors could you help in rerunning this performance test...

compiler-errors · 2024-01-02T15:31:25Z

@bors try @rust-timer queue

bors · 2024-01-02T15:32:35Z

⌛ Trying commit 4fda2d8 with merge 7236802...

…e_2, r=<try> aligns the behavior to that prior to rust-lang#118311 After rust-lang#118311, it seems that due to an oversight some alignments were unintentionally omitted, possibly leading the code into different branches. This PR attempts to restore those alignments and aims to fix the regression reported at rust-lang#118319 (comment)

bors · 2024-01-02T16:59:27Z

☀️ Try build successful - checks-actions
Build commit: 7236802 (7236802464a37483410191928abfd490527a1473)

rust-timer · 2024-01-02T19:10:43Z

Finished benchmarking commit (7236802): comparison URL.

Overall result: no relevant changes - 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-perf -perf-regression

Instruction count

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

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

Cycles

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

Binary size

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

Bootstrap: 666.95s -> 667.828s (0.13%)
Artifact size: 311.74 MiB -> 311.79 MiB (0.01%)

compiler-errors · 2024-01-02T19:46:06Z

@bvanjoi:

Ultimately, I noticed some discrepancies between the usage of movability and tcx.coroutine_kind. These differences can be seen at

Do you mean performance discrepancies? Because there should be no behavioral discrepancies. This is what I meant by #118411 (comment).

Since it's evident that this PR is not going to restore the performance before #118311, is it worth landing this? Does this fix remaining any bugs?

bvanjoi · 2024-01-03T04:50:06Z

Closing it as no substantial help has been provided.....

rustbot assigned b-naber Nov 28, 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 Nov 28, 2023

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

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 28, 2023

pnkfelix mentioned this pull request Nov 28, 2023

merge DefKind::Coroutine into Defkind::Closure #118311

Merged

bvanjoi force-pushed the merge_coroutinue_into_closure_2 branch from 4307541 to 77f9a77 Compare November 29, 2023 16:05

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 Dec 5, 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 Dec 5, 2023

bvanjoi mentioned this pull request Dec 6, 2023

revert for benchmark #118436

Closed

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 Dec 28, 2023

wesleywiser added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 28, 2023

rustbot assigned compiler-errors and unassigned b-naber Dec 28, 2023

compiler-errors reviewed Dec 28, 2023

View reviewed changes

aligns the behavior to that prior to rust-lang#118311

4fda2d8

bvanjoi force-pushed the merge_coroutinue_into_closure_2 branch from 77f9a77 to 4fda2d8 Compare January 2, 2024 14:50

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 Jan 2, 2024

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jan 2, 2024

bvanjoi closed this Jan 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aligns the behavior to that prior to #118311 #118411

aligns the behavior to that prior to #118311 #118411

bvanjoi commented Nov 28, 2023

rustbot commented Nov 28, 2023

compiler-errors commented Nov 28, 2023

This comment has been minimized.

bors commented Nov 28, 2023

bors commented Nov 28, 2023

This comment has been minimized.

rust-timer commented Nov 28, 2023

rustbot commented Nov 29, 2023

bvanjoi commented Nov 29, 2023 •

edited

Loading

bvanjoi commented Nov 29, 2023

compiler-errors commented Dec 5, 2023

This comment has been minimized.

bors commented Dec 5, 2023

bors commented Dec 5, 2023

This comment has been minimized.

rust-timer commented Dec 5, 2023

apiraino commented Dec 28, 2023 •

edited

Loading

petrochenkov commented Dec 28, 2023

compiler-errors commented Dec 28, 2023

compiler-errors commented Dec 28, 2023

compiler-errors Dec 28, 2023

bvanjoi Jan 2, 2024

bvanjoi commented Dec 28, 2023

compiler-errors commented Dec 28, 2023

bvanjoi commented Jan 2, 2024

bvanjoi commented Jan 2, 2024

compiler-errors commented Jan 2, 2024

This comment has been minimized.

bors commented Jan 2, 2024

bors commented Jan 2, 2024

This comment has been minimized.

rust-timer commented Jan 2, 2024

compiler-errors commented Jan 2, 2024 •

edited

Loading

bvanjoi commented Jan 3, 2024

aligns the behavior to that prior to #118311 #118411

aligns the behavior to that prior to #118311 #118411

Conversation

bvanjoi commented Nov 28, 2023

rustbot commented Nov 28, 2023

compiler-errors commented Nov 28, 2023

This comment has been minimized.

bors commented Nov 28, 2023

bors commented Nov 28, 2023

This comment has been minimized.

rust-timer commented Nov 28, 2023

Overall result: no relevant changes - no action needed

rustbot commented Nov 29, 2023

bvanjoi commented Nov 29, 2023 • edited Loading

bvanjoi commented Nov 29, 2023

compiler-errors commented Dec 5, 2023

This comment has been minimized.

bors commented Dec 5, 2023

bors commented Dec 5, 2023

This comment has been minimized.

rust-timer commented Dec 5, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

apiraino commented Dec 28, 2023 • edited Loading

petrochenkov commented Dec 28, 2023

compiler-errors commented Dec 28, 2023

compiler-errors commented Dec 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvanjoi commented Dec 28, 2023

compiler-errors commented Dec 28, 2023

bvanjoi commented Jan 2, 2024

bvanjoi commented Jan 2, 2024

compiler-errors commented Jan 2, 2024

This comment has been minimized.

bors commented Jan 2, 2024

bors commented Jan 2, 2024

This comment has been minimized.

rust-timer commented Jan 2, 2024

Overall result: no relevant changes - no action needed

compiler-errors commented Jan 2, 2024 • edited Loading

bvanjoi commented Jan 3, 2024

bvanjoi commented Nov 29, 2023 •

edited

Loading

apiraino commented Dec 28, 2023 •

edited

Loading

compiler-errors commented Jan 2, 2024 •

edited

Loading