Remove is global hack #116417

ouz-a · 2023-10-04T11:16:41Z

In attempt to fix #114057 we found several issues with how compiler computes layouts, this change removes is_global from and to stop impl from being shadowed.

In depth conversation can be read here https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/Getting.20different.20types.20from.20almost.20same.20inputs

This is a fix candidate opened for performance run.

r? @lcnr

fmease · 2023-10-04T11:33:38Z

@bors try @rust-timer queue

bors · 2023-10-04T11:34:47Z

⌛ Trying commit 56d336a with merge aaa6b9c...

Remove is global hack In attempt to fix rust-lang#114057 we found several issues with how compiler computes layouts, this change removes `is_global` from `and` to stop impl from being shadowed. In depth conversation can be read here https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/Getting.20different.20types.20from.20almost.20same.20inputs This is a fix candidate opened for performance run. r? `@lcnr`

bors · 2023-10-04T12:48:20Z

☀️ Try build successful - checks-actions
Build commit: aaa6b9c (aaa6b9c5084cdaabdcecb0b9fa542444558213f6)

rust-timer · 2023-10-04T14:36:33Z

Finished benchmarking commit (aaa6b9c): 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.4%	[0.2%, 1.6%]	22
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 1.6%]	22

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)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	2
Improvements ✅ (secondary)	-1.3%	[-2.4%, -1.0%]	4
All ❌✅ (primary)	-2.9%	[-2.9%, -2.9%]	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)	1.4%	[1.4%, 1.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[1.4%, 1.4%]	1

Binary size

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

Bootstrap: 622.211s -> 624.134s (0.31%)
Artifact size: 272.03 MiB -> 270.60 MiB (-0.53%)

compiler/rustc_middle/src/ty/mod.rs

compiler-errors · 2023-10-05T18:18:17Z

compiler/rustc_middle/src/ty/mod.rs

    /// Creates a suitable environment in which to perform trait
    /// queries on the given value. When type-checking, this is simply
-    /// the pair of the environment plus value. But when reveal is set to
-    /// All, then if `value` does not reference any type parameters, we will
-    /// pair it with the empty environment. This improves caching and is generally
-    /// invisible.
+    /// the pair of the environment plus value.
    ///
    /// N.B., we preserve the environment when type-checking because it
    /// is possible for the user to have wacky where-clauses like
    /// `where Box<u32>: Copy`, which are clearly never
    /// satisfiable. We generally want to behave as if they were true,
    /// although the surrounding function is never reachable.


This whole message makes no sense anymore.

/// Creates a pair of param-env and value for use in queries.

compiler-errors · 2023-10-05T18:18:42Z

pls amend this commit, then r=me

@bors delegate+

bors · 2023-10-05T18:18:45Z

✌️ @ouz-a, you can now approve this pull request!

If @compiler-errors told you to "r=me" after making some further change, please make that change, then do @bors r=@compiler-errors

ouz-a · 2023-10-05T18:38:43Z

@bors r=compiler-errors

bors · 2023-10-05T18:38:45Z

📌 Commit e30d27b has been approved by compiler-errors

It is now in the queue for this repository.

compiler-errors · 2023-10-05T18:40:35Z

@bors r+ rollup=never

bors · 2023-10-05T18:40:37Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: [perf] Loan liveness frontier slowness evaluation #112432

bors · 2023-10-05T18:40:39Z

📌 Commit e30d27b has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-10-05T19:42:08Z

⌛ Testing commit e30d27b with merge cae0791...

bors · 2023-10-05T21:28:53Z

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

rust-timer · 2023-10-06T02:15:18Z

Finished benchmarking commit (cae0791): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.4%	[0.1%, 1.4%]	22
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-0.9%, -0.2%]	7
All ❌✅ (primary)	0.4%	[0.1%, 1.4%]	22

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)	3.2%	[2.8%, 3.6%]	2
Regressions ❌ (secondary)	4.6%	[4.6%, 4.6%]	1
Improvements ✅ (primary)	-2.8%	[-3.3%, -2.0%]	3
Improvements ✅ (secondary)	-2.4%	[-5.3%, -0.9%]	7
All ❌✅ (primary)	-0.4%	[-3.3%, 3.6%]	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)	0.8%	[0.5%, 1.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.5%, 1.1%]	2

Binary size

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

Bootstrap: 622.141s -> 624.508s (0.38%)
Artifact size: 271.92 MiB -> 270.64 MiB (-0.47%)

nnethercote · 2023-10-06T02:19:20Z

The post-merge perf regression match those seen before merging, which makes it seem like they were expected, though I don't see any discussion in the comments. Is it unavoidable because this is a correctness fix? Any way to mitigate?

compiler-errors · 2023-10-06T02:22:42Z

Sorry, I forgot to link https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/.23116417.3A.20Remove.20is.20global.20hack

T-types came to the (informal) consensus that this hack is not worth the optimization that it affords, since it subtly breaks invariants about the trait solver in functions that are polymorphic over lifetimes (or have higher-ranked trait bounds).

yeet `ReErased` from `HAS_FREE_LOCAL_NAMES` since `ParamEnv::and` hack is dead I always thought this was kinda sketchy, but it was only needed due to the hack removed in rust-lang#116417 r? types

lqd · 2023-10-06T06:03:37Z

Then from above and zulip, the results were expected, @rustbot label: +perf-regression-triaged

yeet `ReErased` from `HAS_FREE_LOCAL_NAMES` since `ParamEnv::and` hack is dead I always thought this was kinda sketchy, but it was only needed due to the hack removed in rust-lang#116417 r? types

Update Rust toolchain from nightly-2023-10-05 to nightly-2023-10-06 without any other source changes. This is an automatically generated pull request. If any of the CI checks fail, manual intervention is required. In such a case, review the changes at https://github.com/rust-lang/rust from rust-lang@2bbb619 up to rust-lang@cae0791. The log for this commit range is: rust-lang@cae0791da4 Auto merge of rust-lang#116417 - ouz-a:trait_type_detective, r=compiler-errors rust-lang@e30d27be00 remove is global hack rust-lang@cdca82c2c8 Auto merge of rust-lang#116455 - matthiaskrgr:rollup-p226a5u, r=matthiaskrgr rust-lang@76d0b794cb Rollup merge of rust-lang#116452 - cjgillot:noassert-erased, r=oli-obk rust-lang@c1c5ab717e Rollup merge of rust-lang#116428 - Alexendoo:note-duplicate-diagnostics, r=compiler-errors,estebank rust-lang@08cc7428d9 Rollup merge of rust-lang#116415 - ouz-a:move_subtyper, r=oli-obk rust-lang@b301bd4220 Rollup merge of rust-lang#116288 - ouz-a:smir_spans, r=spastorino rust-lang@864e5d8d94 Rollup merge of rust-lang#116220 - llogiq:stabilize-option-as-slice, r=BurntSushi rust-lang@3088c4b046 move subtyper change reveal_all rust-lang@14c846cb05 Do not assert that hidden types don't have erased regions. rust-lang@3bcad65fbf Auto merge of rust-lang#103046 - JanBeh:PR_clarify_cmp_terminology, r=workingjubilee rust-lang@cf9fd95b1c Auto merge of rust-lang#114042 - liushuyu:ubuntu/i586-fpmath, r=workingjubilee rust-lang@86b031b734 docs: Correct terminology in std::cmp rust-lang@90f3a6f920 Auto merge of rust-lang#104153 - tspiteri:doc-float-constants, r=workingjubilee rust-lang@5c3a0e932b Auto merge of rust-lang#116427 - cjgillot:no-internal, r=oli-obk rust-lang@a49138e46e impl stable for kinds rust-lang@a79567b01c add span to statements rust-lang@e293927016 Auto merge of rust-lang#116443 - workingjubilee:rollup-r9mh13f, r=workingjubilee rust-lang@4a14a80605 Rollup merge of rust-lang#116432 - notriddle:master, r=fmease rust-lang@d7b02c3d40 Rollup merge of rust-lang#116431 - estebank:issue-80476, r=compiler-errors rust-lang@a9a389cf44 Rollup merge of rust-lang#116429 - fmease:clean-up-struct-field-suggs, r=compiler-errors rust-lang@cfce3a919d Rollup merge of rust-lang#116296 - compiler-errors:default-return, r=estebank rust-lang@ea3454eabb Rollup merge of rust-lang#116223 - catandcoder:master, r=cjgillot rust-lang@5236c8e1fa Auto merge of rust-lang#116273 - compiler-errors:refine2, r=tmandry rust-lang@b781645332 Auto merge of rust-lang#116184 - compiler-errors:afit-lint, r=tmandry rust-lang@5453a9f34d Add a note to duplicate diagnostics rust-lang@afe67fa2ef Auto merge of rust-lang#116370 - nnethercote:more-arena-stuff, r=cjgillot rust-lang@2f5249019e Apply suggestions from code review rust-lang@dd5f26c42d Fix spans for comments in rustfmt rust-lang@1f079cfb44 Point to closure return instead of output if defaulted rust-lang@89b14ae212 Fix clippy rust-lang@137b6d0b01 Point to where missing return type should go rust-lang@a46ccd8d3f Add URL to test case issues rust-lang@9266270ef5 Rename issue-\d+.rs tests to have meaningful names rust-lang@041e54bd92 Tweak wording of E0562 rust-lang@867cc41b5b clean up struct field suggestions rust-lang@a198aff4a4 Add `crate_name` to test so that it can be renamed rust-lang@e63d19c4dd Remove mir::LocalDecl::internal. rust-lang@f44d116e1f Fix misuses of a vs an rust-lang@a2051dd578 Optimize some `alloc_from_iter` call sites. rust-lang@816383c60d Remove the `TypedArena::alloc_from_iter` specialization. rust-lang@c373d206cd Address review nits rust-lang@90dfa24415 Only reachable traits rust-lang@afea0b4eab Fill in prose to describe the `async_fn_in_trait` lint rust-lang@28d58f6524 Bless tests rust-lang@ec79720c1e Add async_fn_in_trait lint rust-lang@999a354a81 add span to terminator rust-lang@9130484db9 create localdecl and add span to it rust-lang@06d9602d33 Only trigger refine lint on reachable traits rust-lang@702da3b89c stabilize `Option::as_`(`mut_`)`slice` rust-lang@40a52cf55c core library: Disable fpmath tests for i386 ... rust-lang@c953b6c014 doc: expand description for f32 and f64 associated constants Co-authored-by: celinval <[email protected]>

rustbot assigned lcnr Oct 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 Oct 4, 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 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 4, 2023

lcnr added the I-types-nominated Nominated for discussion during a types team meeting. label Oct 4, 2023

compiler-errors reviewed Oct 4, 2023

View reviewed changes

compiler/rustc_middle/src/ty/mod.rs Outdated Show resolved Hide resolved

ouz-a force-pushed the trait_type_detective branch from c49195b to 712a894 Compare October 5, 2023 07:36

compiler-errors approved these changes Oct 5, 2023

View reviewed changes

remove is global hack

e30d27b

ouz-a force-pushed the trait_type_detective branch from 712a894 to e30d27b Compare October 5, 2023 18:38

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

ouz-a mentioned this pull request Oct 5, 2023

Trait's types could not be determined #114057

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 5, 2023

bors merged commit cae0791 into rust-lang:master Oct 5, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Oct 5, 2023

compiler-errors mentioned this pull request Oct 6, 2023

yeet ReErased from HAS_FREE_LOCAL_NAMES since ParamEnv::and hack is dead #116467

Closed

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 6, 2023

pvdrz mentioned this pull request Oct 6, 2023

Fix divergence from upstream master ferrocene/ferrocene#23

Merged

matthiaskrgr mentioned this pull request Oct 7, 2023

ice: slices are not scalars #116513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove is global hack #116417

Remove is global hack #116417

ouz-a commented Oct 4, 2023

fmease commented Oct 4, 2023

This comment has been minimized.

bors commented Oct 4, 2023

bors commented Oct 4, 2023

This comment has been minimized.

rust-timer commented Oct 4, 2023

compiler-errors Oct 5, 2023

compiler-errors commented Oct 5, 2023

bors commented Oct 5, 2023

ouz-a commented Oct 5, 2023

bors commented Oct 5, 2023

compiler-errors commented Oct 5, 2023

bors commented Oct 5, 2023

bors commented Oct 5, 2023

bors commented Oct 5, 2023

bors commented Oct 5, 2023

rust-timer commented Oct 6, 2023

nnethercote commented Oct 6, 2023

compiler-errors commented Oct 6, 2023

lqd commented Oct 6, 2023

Remove is global hack #116417

Remove is global hack #116417

Conversation

ouz-a commented Oct 4, 2023

fmease commented Oct 4, 2023

This comment has been minimized.

bors commented Oct 4, 2023

bors commented Oct 4, 2023

This comment has been minimized.

rust-timer commented Oct 4, 2023

Overall result: ❌ regressions - ACTION NEEDED

Choose a reason for hiding this comment

compiler-errors commented Oct 5, 2023

bors commented Oct 5, 2023

ouz-a commented Oct 5, 2023

bors commented Oct 5, 2023

compiler-errors commented Oct 5, 2023

bors commented Oct 5, 2023

bors commented Oct 5, 2023

bors commented Oct 5, 2023

bors commented Oct 5, 2023

rust-timer commented Oct 6, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

nnethercote commented Oct 6, 2023

compiler-errors commented Oct 6, 2023

lqd commented Oct 6, 2023