Explain revealing of opaque types in layout_of ParamEnv #115762

oli-obk · 2023-09-11T17:31:23Z

~~I feel like layout_of is doing too many things at once, and I don't really know why. It could allow us to if callers could decide whether to reveal opaque types.~~

Looks like this also exists as a performance optimization. While we could probably figure out a way to do this, all the ones I came up with are fragile as layout_of callers now suddenly need to be careful what ParamEnv they pass in.

rustbot · 2023-09-11T17:31:33Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors · 2023-09-11T17:33:11Z

compiler/rustc_middle/src/ty/layout.rs

@@ -324,6 +324,7 @@ impl<'tcx> SizeSkeleton<'tcx> {
        param_env: ty::ParamEnv<'tcx>,
    ) -> Result<SizeSkeleton<'tcx>, &'tcx LayoutError<'tcx>> {
        debug_assert!(!ty.has_non_region_infer());
+        let param_env = param_env.with_reveal_all_normalized(tcx);


Ok yeah, this preserves the revealing behavior of size computation. I think that's why more tests don't explode, makes sense.

yea, I had lots of tests explode, and didn't look too deeply, but assumed I could write a stable test that would break

oli-obk · 2023-09-11T17:38:45Z

@bors try @rust-timer queue

bors · 2023-09-11T17:38:55Z

⌛ Trying commit 21e8d63 with merge ec12977...

Avoid revealing in layout_of r? `@compiler-errors` I feel like `layout_of` is doing too many things at once, and I don't really know why. It could allow us to if callers could decide whether to reveal opaque types.

bors · 2023-09-11T18:50:08Z

☀️ Try build successful - checks-actions
Build commit: ec12977 (ec12977d578aa479facdaead3d91cc98d6a74af4)

rust-timer · 2023-09-12T05:35:15Z

Finished benchmarking commit (ec12977): 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)	0.5%	[0.2%, 0.8%]	11
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	4
All ❌✅ (primary)	0.5%	[0.2%, 0.8%]	11

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.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	3.7%	[1.5%, 5.9%]	2
Improvements ✅ (primary)	-1.4%	[-1.4%, -1.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-1.4%, 0.4%]	2

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

Binary size

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

Bootstrap: 629.503s -> 632.559s (0.49%)
Artifact size: 317.74 MiB -> 317.72 MiB (-0.01%)

compiler-errors · 2023-09-16T19:53:34Z

Need to bless tests

@rustbot author

rustbot · 2023-09-18T10:11:53Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

oli-obk · 2023-09-18T10:18:30Z

hmm... looks like we got a lot more layout_of executions now. I guess that makes sense, as we now have various cases where we use the tcx.param_env(def_id) and will at some point also use the tcx.param_env_reveal_all(def_id) for the same values. We're not doing the const-eval style optimization of invoking Reveal::UserFacing and only doing the full thing if that fails.

I'll open a separate PR to try out the const eval trick on layout_of

oli-obk · 2023-09-18T10:39:54Z

Nevermind, that change isn't possible without also doing this PR, so trying it out here

@bors try @rust-timer queue

bors · 2023-09-18T10:40:03Z

⌛ Trying commit 28bfd7a with merge e9ce6f9...

Avoid revealing in layout_of r? `@compiler-errors` I feel like `layout_of` is doing too many things at once, and I don't really know why. It could allow us to if callers could decide whether to reveal opaque types.

bors · 2023-09-18T11:49:44Z

☀️ Try build successful - checks-actions
Build commit: e9ce6f9 (e9ce6f9977710d60c6ebb1ec454bc6a49b032d03)

rust-timer · 2023-09-18T13:26:30Z

Finished benchmarking commit (e9ce6f9): 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)	0.5%	[0.3%, 0.8%]	30
Regressions ❌ (secondary)	0.8%	[0.3%, 1.2%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	4
All ❌✅ (primary)	0.5%	[0.3%, 0.8%]	30

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)	1.1%	[0.9%, 1.4%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-0.7%, -0.6%]	2
All ❌✅ (primary)	1.1%	[0.9%, 1.4%]	5

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: 633.305s -> 636.592s (0.52%)
Artifact size: 317.99 MiB -> 318.00 MiB (0.00%)

oli-obk · 2023-09-18T15:14:28Z

I guess there's nothing to be done here. This is an optimization, so my original reasoning about there being no reason for it is wrong

oli-obk · 2023-09-18T15:19:53Z

@bors rollup

@rustbot ready

compiler-errors · 2023-09-18T15:22:51Z

@bors r+

bors · 2023-09-18T15:22:53Z

📌 Commit ee59531 has been approved by compiler-errors

It is now in the queue for this repository.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#109409 (Add `minmax{,_by,_by_key}` functions to `core::cmp`) - rust-lang#115494 (get rid of duplicate primitive_docs) - rust-lang#115663 (ci: actions/checkout@v3 to actions/checkout@v4) - rust-lang#115762 (Explain revealing of opaque types in layout_of ParamEnv) - rust-lang#115891 (simplify inject_impl_of_structural_trait) - rust-lang#115932 (Expand infra-ci reviewer list) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#115762 - oli-obk:early_const_prop_lint2, r=compiler-errors Explain revealing of opaque types in layout_of ParamEnv r? `@compiler-errors` ~~I feel like `layout_of` is doing too many things at once, and I don't really know why. It could allow us to if callers could decide whether to reveal opaque types.~~ Looks like this also exists as a performance optimization. While we could probably figure out a way to do this, all the ones I came up with are fragile as `layout_of` callers now suddenly need to be careful what ParamEnv they pass in.

rustbot assigned compiler-errors Sep 11, 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 Sep 11, 2023

compiler-errors reviewed Sep 11, 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 Sep 11, 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 Sep 12, 2023

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 Sep 16, 2023

oli-obk force-pushed the early_const_prop_lint2 branch from 21e8d63 to f316e44 Compare September 18, 2023 10:11

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 Sep 18, 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 Sep 18, 2023

Explain with_reveal_all_normalized usage

ee59531

oli-obk force-pushed the early_const_prop_lint2 branch from 28bfd7a to ee59531 Compare September 18, 2023 15:18

oli-obk changed the title ~~Avoid revealing in layout_of~~ Explain revealing of opaque types in layout_of ParamEnv Sep 18, 2023

rustbot 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 Sep 18, 2023

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 Sep 18, 2023

matthiaskrgr mentioned this pull request Sep 18, 2023

Rollup of 6 pull requests #115940

Merged

bors merged commit e27c3e9 into rust-lang:master Sep 18, 2023
11 checks passed

rustbot added this to the 1.74.0 milestone Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explain revealing of opaque types in layout_of ParamEnv #115762

Explain revealing of opaque types in layout_of ParamEnv #115762

oli-obk commented Sep 11, 2023 •

edited

Loading

rustbot commented Sep 11, 2023

compiler-errors Sep 11, 2023

oli-obk Sep 11, 2023

oli-obk commented Sep 11, 2023

This comment has been minimized.

bors commented Sep 11, 2023

This comment has been minimized.

bors commented Sep 11, 2023

This comment has been minimized.

rust-timer commented Sep 12, 2023

compiler-errors commented Sep 16, 2023

rustbot commented Sep 18, 2023

oli-obk commented Sep 18, 2023

oli-obk commented Sep 18, 2023

This comment has been minimized.

bors commented Sep 18, 2023

This comment has been minimized.

bors commented Sep 18, 2023

This comment has been minimized.

rust-timer commented Sep 18, 2023

oli-obk commented Sep 18, 2023

oli-obk commented Sep 18, 2023

compiler-errors commented Sep 18, 2023

bors commented Sep 18, 2023

Explain revealing of opaque types in layout_of ParamEnv #115762

Explain revealing of opaque types in layout_of ParamEnv #115762

Conversation

oli-obk commented Sep 11, 2023 • edited Loading

rustbot commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Sep 11, 2023

This comment has been minimized.

bors commented Sep 11, 2023

This comment has been minimized.

bors commented Sep 11, 2023

This comment has been minimized.

rust-timer commented Sep 12, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

compiler-errors commented Sep 16, 2023

rustbot commented Sep 18, 2023

oli-obk commented Sep 18, 2023

oli-obk commented Sep 18, 2023

This comment has been minimized.

bors commented Sep 18, 2023

This comment has been minimized.

bors commented Sep 18, 2023

This comment has been minimized.

rust-timer commented Sep 18, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

oli-obk commented Sep 18, 2023

oli-obk commented Sep 18, 2023

compiler-errors commented Sep 18, 2023

bors commented Sep 18, 2023

oli-obk commented Sep 11, 2023 •

edited

Loading