Remove allow(rustc::potential_query_instability) from rustc_ast_lowering #104330

CastilloDel · 2022-11-12T18:11:03Z

Related to #84447.

@cjgillot Thanks for helping me!

rustbot · 2022-11-12T18:11:11Z

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

cjgillot · 2022-11-12T19:01:42Z

compiler/rustc_ast_lowering/src/item.rs

-            self.owners[def_id] = info;
+            if let hir::MaybeOwner::NonOwner(_) = self.owners[def_id] {
+                self.owners[def_id] = info;
+            }


The logic is reversed here.
Phantom means nothing has been assigned yet.
An Owner can overwrite a NonOwner, and a NonOwner does not overwrite to an Owner.
If we try to overwrite an Owner with an Owner, or a NonOwner with a NonOwner, that's a bug.

Is it better now?

Actually, as the test suite passes with the debug assert just above, let's keep the simple assignment there was originally. Sorry for the back-and-forth.

cjgillot · 2022-11-13T15:15:15Z

Just in case:
@bors try @rust-timer queue

bors · 2022-11-13T15:15:24Z

⌛ Trying commit 6c711ad6a9d5ae66ddd63eb4ff8eb48e4ec886f6 with merge f3caf91746d059764528a52ecc05c4498425a7ff...

bors · 2022-11-13T17:55:48Z

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

rust-timer · 2022-11-13T19:13:52Z

Finished benchmarking commit (f3caf91746d059764528a52ecc05c4498425a7ff): comparison URL.

Overall result: ❌✅ regressions and improvements - 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-review -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
Regressions ❌ (secondary)	1.3%	[1.3%, 1.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.3%]	2
All ❌✅ (primary)	-	-	0

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)	-1.4%	[-2.0%, -0.9%]	2
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-1.4%	[-2.0%, -0.9%]	2

Cycles

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

CastilloDel · 2022-11-14T11:11:58Z

A regression in a secondary benchmark doesn't seem so bad. I will change the Vec to a SmallVec to test if the performance improves.

Request a benchmark, please!

cjgillot · 2022-11-14T17:38:57Z

@bors try @rust-timer queue

bors · 2022-11-14T17:39:06Z

⌛ Trying commit 8d360aa80121fcaff4196ada0e92bbefca933b5d with merge b781f49a47f316f7a4763b82f0fe195a4677a72a...

cjgillot

One nit, r=me on the best of Vec/SmallVec.

cjgillot · 2022-11-13T15:14:08Z

compiler/rustc_ast_lowering/src/item.rs

-            self.owners[def_id] = info;
+            if let hir::MaybeOwner::NonOwner(_) = self.owners[def_id] {
+                self.owners[def_id] = info;
+            }


Actually, as the test suite passes with the debug assert just above, let's keep the simple assignment there was originally. Sorry for the back-and-forth.

compiler/rustc_ast_lowering/src/item.rs

bors · 2022-11-14T20:19:27Z

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

rust-timer · 2022-11-14T21:37:00Z

Finished benchmarking commit (b781f49a47f316f7a4763b82f0fe195a4677a72a): comparison URL.

Overall result: ❌ regressions - 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-review -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
Regressions ❌ (secondary)	0.2%	[0.1%, 0.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	2.4%	[1.4%, 3.4%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-2.1%, -0.9%]	2
All ❌✅ (primary)	-	-	0

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

CastilloDel · 2022-11-14T23:53:54Z

Not sure which one is best overall. The two improvements we had before are gone, and now we have one more regression, but they are smaller. Should we try another SmallVec size? and if not which version should we use?

cjgillot · 2022-11-15T12:27:14Z

Not sure which one is best overall. The two improvements we had before are gone, and now we have one more regression, but they are smaller. Should we try another SmallVec size? and if not which version should we use?

The Vec version was good enough. Picking over fractions of a percent over 2 benchmarks is not worth it.

Manishearth · 2022-11-16T17:45:34Z

@bors p=1

going to close the tree for non-nevers for a while so they can drain out

bors · 2022-11-17T16:47:51Z

⌛ Testing commit 5572263 with merge 80ac74d4fc73147a5458c9580a8e40651c379363...

bors · 2022-11-17T17:15:25Z

💔 Test failed - checks-actions

CastilloDel · 2022-11-17T17:30:32Z

I don't understand the error :/

rust-log-analyzer · 2022-11-17T17:40:19Z

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

---- [ui] src/test/ui/array-slice-vec/box-of-array-of-drop-2.rs stdout ----

error: test run failed!
status: exit status: 101
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "run" "0" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/array-slice-vec/box-of-array-of-drop-2/a"
--- stdout -------------------------------
uploaded "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/array-slice-vec/box-of-array-of-drop-2/a", waiting for result
--- stderr -------------------------------
thread 'main' panicked at 'client.into_inner() failed with Connection reset by peer (os error 104)', src/tools/remote-test-client/src/main.rs:306:22
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
------------------------------------------

cjgillot · 2022-11-17T17:49:39Z

@bors retry

bors · 2022-11-17T20:54:55Z

⌛ Testing commit 5572263 with merge c0bec3a826c1ac2582c6d9c2912efab71a5762ba...

bors · 2022-11-18T01:09:55Z

💥 Test timed out

rust-log-analyzer · 2022-11-18T04:03:34Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

cjgillot · 2022-11-18T11:27:04Z

@bors retry

bors · 2022-11-18T13:12:22Z

⌛ Testing commit 5572263 with merge fd3bfb3...

bors · 2022-11-18T16:12:22Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing fd3bfb3 to master...

rust-timer · 2022-11-18T18:16:36Z

Finished benchmarking commit (fd3bfb3): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-1.3%, -0.3%]	6
All ❌✅ (primary)	-	-	0

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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.6%	[-4.6%, -4.6%]	1
All ❌✅ (primary)	0.5%	[0.5%, 0.5%]	1

Cycles

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

Remove allow(rustc::potential_query_instability) from rustc_ast_lowering Related to rust-lang#84447. `@cjgillot` Thanks for helping me!

rustbot assigned fee1-dead Nov 12, 2022

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 12, 2022