ast_lowering: Make some asserts in ID generation debug only. #119234

aDotInTheVoid · 2023-12-22T23:37:35Z

Ran into this code, as was kinda surprised these are still here in release. These methods seem hot.

aDotInTheVoid · 2023-12-22T23:42:33Z

bors · 2023-12-22T23:43:42Z

⌛ Trying commit 0c7ec74 with merge 3927ced...

…r=<try> ast_lowering: Make some asserts debug only. Ran into this code, as was kinda surprised these are still here in release. These methods seem hot. r? `@ghost` `@bors` try `@rust-timer` queue

bors · 2023-12-23T01:09:45Z

☀️ Try build successful - checks-actions
Build commit: 3927ced (3927ced0b6d54dc86c2152de7f072594945f73e2)

rust-timer · 2023-12-23T06:28:52Z

Finished benchmarking commit (3927ced): comparison URL.

Overall result: ✅ 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-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
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	3
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	3

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

Cycles

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

Binary size

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

Bootstrap: 672.389s -> 670.281s (-0.31%)
Artifact size: 312.74 MiB -> 312.74 MiB (-0.00%)

aDotInTheVoid · 2023-12-23T10:30:37Z

r? compiler

rustbot · 2023-12-23T10:31:52Z

Failed to set assignee to ghost: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

compiler-errors

One nit -- I think if we use debug_assert_eq we get nicer diagnostics

compiler/rustc_ast_lowering/src/lib.rs

petrochenkov · 2023-12-23T21:15:16Z

compiler/rustc_ast_lowering/src/lib.rs

-        assert!(
-            self.opt_local_def_id(node_id).is_none(),
+        debug_assert_eq!(
+            self.opt_local_def_id(node_id),


Not very hot (only called for definitions created by syntactic sugar), but the assert performs a non-trivial calculation.
Not sure about usefulness of this assert.

petrochenkov · 2023-12-23T21:17:08Z

compiler/rustc_ast_lowering/src/lib.rs

@@ -702,7 +703,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
    /// properly. Calling the method twice with the same `NodeId` is fine though.
    #[instrument(level = "debug", skip(self), ret)]
    fn lower_node_id(&mut self, ast_node_id: NodeId) -> hir::HirId {
-        assert_ne!(ast_node_id, DUMMY_NODE_ID);
+        debug_assert_ne!(ast_node_id, DUMMY_NODE_ID);


Hot (called for most nodes in code), but the assert is trivial.
This asserts looks pretty useful.

petrochenkov · 2023-12-23T21:18:13Z

compiler/rustc_ast_lowering/src/lib.rs

@@ -717,7 +718,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                v.insert(local_id);
                self.item_local_id_counter.increment_by(1);

-                assert_ne!(local_id, hir::ItemLocalId::new(0));
+                debug_assert_ne!(local_id, hir::ItemLocalId::new(0));


Hot (called for most nodes in code), but the assert is trivial.
Not sure about usefulness of this assert.

petrochenkov · 2023-12-23T21:18:43Z

compiler/rustc_ast_lowering/src/lib.rs

@@ -736,7 +737,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
    fn next_id(&mut self) -> hir::HirId {
        let owner = self.current_hir_id_owner;
        let local_id = self.item_local_id_counter;
-        assert_ne!(local_id, hir::ItemLocalId::new(0));
+        debug_assert_ne!(local_id, hir::ItemLocalId::new(0));


Not very hot (only called for nodes created by syntactic sugar), and the assert is trivial.
Not sure about usefulness of this assert.

petrochenkov · 2023-12-23T21:19:43Z

I'd generally prefer to keep useful asserts unless they provably regress performance.
Leaving this to r? @compiler-errors

compiler-errors · 2023-12-25T22:04:51Z

I agree with petrochenkov's thoughts above. Could we restore the ones that petrochenkov said were useful assertions?

aDotInTheVoid · 2023-12-26T00:56:14Z

@bors try @rust-timer queue

bors · 2023-12-26T00:57:26Z

⌛ Trying commit f3a7297 with merge 0d3dccf...

…r=<try> ast_lowering: Make some asserts in ID generation debug only. Ran into this code, as was kinda surprised these are still here in release. These methods seem hot. r? `@ghost` `@bors` try `@rust-timer` queue

rust-timer · 2023-12-26T06:06:23Z

Finished benchmarking commit (0d3dccf): 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

Warning ⚠: The following benchmark(s) failed to build:

cargo-0.60.0
clap-3.1.6
cranelift-codegen-0.82.1

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.8%	[0.6%, 15.8%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.8%	[0.6%, 15.8%]	4

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.0%	[3.0%, 3.0%]	1
Regressions ❌ (secondary)	3.3%	[3.3%, 3.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[3.0%, 3.0%]	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)	5.4%	[0.9%, 13.5%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	5.4%	[0.9%, 13.5%]	3

Binary size

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

Bootstrap: 669.724s -> 668.771s (-0.14%)
Artifact size: 312.56 MiB -> 312.53 MiB (-0.01%)

aDotInTheVoid · 2023-12-26T13:00:21Z

@bors try @rust-timer queue

bors · 2023-12-26T13:01:31Z

⌛ Trying commit 03be3e3 with merge 970345b...

…r=<try> ast_lowering: Make some asserts in ID generation debug only. Ran into this code, as was kinda surprised these are still here in release. These methods seem hot. r? `@ghost` `@bors` try `@rust-timer` queue

bors · 2023-12-26T14:28:57Z

☀️ Try build successful - checks-actions
Build commit: 970345b (970345bf0ea32af095b9173fa6ba4b24098c52b0)

rust-timer · 2023-12-26T17:04:56Z

Finished benchmarking commit (970345b): 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)	4.7%	[4.7%, 4.7%]	1
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.2%	[-4.1%, -2.2%]	2
All ❌✅ (primary)	4.7%	[4.7%, 4.7%]	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: 671.25s -> 671.556s (0.05%)
Artifact size: 312.51 MiB -> 312.53 MiB (0.00%)

aDotInTheVoid · 2023-12-26T21:48:42Z

@bors try @rust-timer queue

bors · 2023-12-26T21:49:53Z

⌛ Trying commit 93781e5 with merge 7837f42...

…r=<try> ast_lowering: Make some asserts in ID generation debug only. Ran into this code, as was kinda surprised these are still here in release. These methods seem hot. r? `@ghost` `@bors` try `@rust-timer` queue

bors · 2023-12-26T23:17:29Z

☀️ Try build successful - checks-actions
Build commit: 7837f42 (7837f4252d5745602e3a1b228d95703aa9f78437)

bors · 2023-12-26T23:17:29Z

☀️ Try build successful - checks-actions
Build commit: 7837f42 (7837f4252d5745602e3a1b228d95703aa9f78437)

rust-timer · 2023-12-27T00:37:30Z

Finished benchmarking commit (7837f42): 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.6%	[-2.6%, -2.6%]	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: 670.702s -> 671.644s (0.14%)
Artifact size: 312.43 MiB -> 312.40 MiB (-0.01%)

Dylan-DPC · 2024-04-21T13:31:04Z

@aDotInTheVoid any updates on this?

compiler-errors · 2024-04-21T14:30:44Z

Not certain that this work is up to date anymore

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 Dec 22, 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 Dec 22, 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 Dec 23, 2023

rustbot assigned petrochenkov Dec 23, 2023

aDotInTheVoid changed the title ~~ast_lowering: Make some asserts debug only.~~ ast_lowering: Make some asserts in ID generation debug only. Dec 23, 2023

compiler-errors approved these changes Dec 23, 2023

View reviewed changes

compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved

rust-cloud-vms bot force-pushed the ast-lowering-asserts branch from 0c7ec74 to bfbc952 Compare December 23, 2023 18:38

petrochenkov reviewed Dec 23, 2023

View reviewed changes

rustbot assigned compiler-errors and unassigned petrochenkov Dec 23, 2023

compiler-errors 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 25, 2023

rust-cloud-vms bot force-pushed the ast-lowering-asserts branch from bfbc952 to f3a7297 Compare December 26, 2023 00:53

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

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 26, 2023

rust-cloud-vms bot force-pushed the ast-lowering-asserts branch from f3a7297 to 03be3e3 Compare December 26, 2023 12:58

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

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

ast_lowering: Make some asserts debug only.

93781e5

rust-cloud-vms bot force-pushed the ast-lowering-asserts branch from 03be3e3 to 93781e5 Compare December 26, 2023 21:46

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

compiler-errors closed this Apr 21, 2024

aDotInTheVoid deleted the ast-lowering-asserts branch April 22, 2024 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ast_lowering: Make some asserts in ID generation debug only. #119234

ast_lowering: Make some asserts in ID generation debug only. #119234

aDotInTheVoid commented Dec 22, 2023

aDotInTheVoid commented Dec 22, 2023

This comment has been minimized.

bors commented Dec 22, 2023

bors commented Dec 23, 2023

This comment has been minimized.

rust-timer commented Dec 23, 2023

aDotInTheVoid commented Dec 23, 2023

rustbot commented Dec 23, 2023

compiler-errors left a comment

petrochenkov Dec 23, 2023

petrochenkov Dec 23, 2023

petrochenkov Dec 23, 2023

petrochenkov Dec 23, 2023

petrochenkov commented Dec 23, 2023

compiler-errors commented Dec 25, 2023

aDotInTheVoid commented Dec 26, 2023

This comment has been minimized.

bors commented Dec 26, 2023

rust-timer commented Dec 26, 2023

aDotInTheVoid commented Dec 26, 2023

This comment has been minimized.

bors commented Dec 26, 2023

bors commented Dec 26, 2023

This comment has been minimized.

rust-timer commented Dec 26, 2023

aDotInTheVoid commented Dec 26, 2023

This comment has been minimized.

bors commented Dec 26, 2023

bors commented Dec 26, 2023

bors commented Dec 26, 2023

This comment has been minimized.

rust-timer commented Dec 27, 2023

Dylan-DPC commented Apr 21, 2024

compiler-errors commented Apr 21, 2024 •

edited

ast_lowering: Make some asserts in ID generation debug only. #119234

ast_lowering: Make some asserts in ID generation debug only. #119234

Conversation

aDotInTheVoid commented Dec 22, 2023

aDotInTheVoid commented Dec 22, 2023

This comment has been minimized.

bors commented Dec 22, 2023

bors commented Dec 23, 2023

This comment has been minimized.

rust-timer commented Dec 23, 2023

Overall result: ✅ improvements - no action needed

aDotInTheVoid commented Dec 23, 2023

rustbot commented Dec 23, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Dec 23, 2023

compiler-errors commented Dec 25, 2023

aDotInTheVoid commented Dec 26, 2023

This comment has been minimized.

bors commented Dec 26, 2023

rust-timer commented Dec 26, 2023

Overall result: ❌ regressions - ACTION NEEDED

aDotInTheVoid commented Dec 26, 2023

This comment has been minimized.

bors commented Dec 26, 2023

bors commented Dec 26, 2023

This comment has been minimized.

rust-timer commented Dec 26, 2023

Overall result: no relevant changes - no action needed

aDotInTheVoid commented Dec 26, 2023

This comment has been minimized.

bors commented Dec 26, 2023

bors commented Dec 26, 2023

bors commented Dec 26, 2023

This comment has been minimized.

rust-timer commented Dec 27, 2023

Overall result: no relevant changes - no action needed

Dylan-DPC commented Apr 21, 2024

compiler-errors commented Apr 21, 2024 • edited

compiler-errors commented Apr 21, 2024 •

edited