Alloc `hir::Lit` in an arena to remove the destructor from `Expr` #109588

Noratrieb · 2023-03-25T10:20:33Z

This allows allocating Exprs into a dropless arena, which is useful for using length prefixed thing slices in HIR, since these can only be allocated in the dropless arena and not in a typed arena.

rustbot · 2023-03-25T10:20:39Z

r? @lcnr

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

Noratrieb · 2023-03-25T10:20:46Z

@bors try @rust-timer queue

bors · 2023-03-25T10:20:55Z

⌛ Trying commit 9985737 with merge a34861623e5b0d2dce2dc5e49e4c4a7e6737ddda...

Noratrieb · 2023-03-25T10:21:23Z

compiler/rustc_hir/src/hir.rs

+        };
+    }
+
+    static_assert_no_destructor!(Expr<'_>);


I'm not sure whether this is a useful thing to have, but I used to to make sure that my code was correct and didn't want to immediately delete it

maybe we should assert that all hir arena_types are dropless?

this is kinda what #109590 does actually

yeah, I'll just remove the assertion here, makes no sense to land, we should just land the other PR afterwards

bors · 2023-03-25T12:00:15Z

💔 Test failed - checks-actions

rustbot · 2023-03-25T12:27:33Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Noratrieb · 2023-03-25T12:28:04Z

@bors try @rust-timer queue

bors · 2023-03-25T12:28:12Z

⌛ Trying commit a14e95ba5779f7334f8f3d1850ab23872ead4933 with merge d329a0a3cf1266923851345bd53ba5a5da70808c...

bors · 2023-03-25T14:23:20Z

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

bors · 2023-03-25T14:23:20Z

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

rust-timer · 2023-03-25T15:50:09Z

Finished benchmarking commit (d329a0a3cf1266923851345bd53ba5a5da70808c): 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.6%	[0.6%, 0.6%]	2
Regressions ❌ (secondary)	0.5%	[0.3%, 0.6%]	4
Improvements ✅ (primary)	-0.7%	[-1.1%, -0.4%]	5
Improvements ✅ (secondary)	-0.6%	[-1.1%, -0.3%]	13
All ❌✅ (primary)	-0.3%	[-1.1%, 0.6%]	7

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.4%	[2.6%, 4.6%]	9
Regressions ❌ (secondary)	3.0%	[0.6%, 7.6%]	127
Improvements ✅ (primary)	-2.3%	[-2.6%, -2.0%]	3
Improvements ✅ (secondary)	-1.9%	[-3.8%, -0.7%]	5
All ❌✅ (primary)	2.0%	[-2.6%, 4.6%]	12

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)	2.3%	[0.4%, 3.1%]	6
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-2.1%	[-3.0%, -0.5%]	3
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Noratrieb · 2023-03-25T15:58:55Z

It got generally a little faster, but max rss went up quite a bit for very literal heavy stress tests like deep-vector. Maybe LitKinds should be interned? Not sure whether this is worth worrying about though, the programs are really extreme and the absolute numbers are probably still pretty low.

Noratrieb · 2023-04-16T15:21:24Z

The follow up PR got rid of the regressions: #109590 (comment)

Oh wait, I misinterpreted this PR's relationship with the other one.

@Nilstrieb, do you want to land this by itself? Or should we just close it and I can review the other one I guess?

apparently these "regressions" are just noise (as usual) so I'll just merge this one on its own, it's a clear improvement.

Noratrieb · 2023-04-16T15:34:56Z

@bors r=compiler-errors

bors · 2023-04-16T15:34:59Z

📌 Commit e616391a06c8187b92ab3d1effd23738abc90a9e has been approved by compiler-errors

It is now in the queue for this repository.

Noratrieb · 2023-04-16T15:35:32Z

@bors r-
I wanted to remove an assertion

This allows allocating `Expr`s into a dropless arena, which is useful for using length prefixed thing slices in HIR, since these can only be allocated in the dropless arena and not in a typed arena. This is something I'm working on.

Noratrieb · 2023-04-16T15:42:19Z

@bors r+

bors · 2023-04-16T15:42:21Z

📌 Commit 34ed5c3 has been approved by Nilstrieb

It is now in the queue for this repository.

Noratrieb · 2023-04-16T15:42:31Z

@bors r=compiler-errors

bors · 2023-04-16T15:42:33Z

💡 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: bolt: remove deprecated option value for split-functions #110403

bors · 2023-04-16T15:42:34Z

📌 Commit 34ed5c3 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-04-17T05:12:15Z

⌛ Testing commit 34ed5c3 with merge 53ac4f8...

bors · 2023-04-17T07:24:54Z

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

rust-timer · 2023-04-17T08:42:57Z

Finished benchmarking commit (53ac4f8): 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)	1.6%	[0.7%, 2.7%]	4
Regressions ❌ (secondary)	6.3%	[5.0%, 8.1%]	6
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.1%]	3
Improvements ✅ (secondary)	-0.6%	[-2.4%, -0.3%]	21
All ❌✅ (primary)	0.7%	[-0.6%, 2.7%]	7

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.5%	[3.1%, 3.9%]	2
Regressions ❌ (secondary)	3.6%	[2.6%, 5.3%]	13
Improvements ✅ (primary)	-3.5%	[-3.6%, -3.4%]	2
Improvements ✅ (secondary)	-2.9%	[-5.2%, -1.4%]	10
All ❌✅ (primary)	0.0%	[-3.6%, 3.9%]	4

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

Noratrieb · 2023-04-17T09:01:24Z

This was green before the merge and this doesn't touch any type system stuff (which cranelift-codegen and keccak hit). Ran a cachegrind anyways

1,361,808,876  ???:<rustc_data_structures::obligation_forest::ObligationForest<rustc_trait_selection::traits::fulfill::PendingPredicateObligation>>::process_obligations::<rustc_trait_selection::traits::fulfill::FulfillProcessor>

The only regression for keccak. Those noise levels are absolutely crazy. I'm not gonna mark it as triaged to make sure that it's triaged appropriately but yeah, no concern about this PR.

nnethercote · 2023-04-17T11:53:47Z

Those noise levels are absolutely crazy.

Can you clarify? keccak has been very noisy at times in the past, but recently it's been stable.

Noratrieb · 2023-04-17T12:07:38Z

Well, the 8% here (which has to be noise, given that this was neutral before and also doesn't touch anything related) is pretty high

lqd · 2023-04-17T20:18:27Z

Noise seems likely, starting from this PR's parent about MIR inlining. Things could be on the way down to the previous steady state, hopefully.

rylev · 2023-04-19T08:17:35Z

Does look like something has made keccak noisy again. The bump starts with this PR but I suppose that could be due to the the previous change kicking off some bimodal behaivor.

Noratrieb · 2023-04-19T09:20:47Z

@rylev it was very likely the parent of this PR, #109247
that change inlining which keccak is very sensitive to

Impl `Copy` for most HIR types This simplifies the invocation of the `arena_types` macro and probably makes working with HIR nicer in general. Based on rust-lang#109588

rustbot assigned lcnr Mar 25, 2023

rustbot added S-waiting-on-review T-compiler labels Mar 25, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf label Mar 25, 2023

Noratrieb commented Mar 25, 2023

View reviewed changes

Noratrieb mentioned this pull request Mar 25, 2023

Impl Copy for most HIR types #109590

Merged

bors added S-waiting-on-author and removed S-waiting-on-review labels Mar 25, 2023

rust-cloud-vms bot force-pushed the dropless-expr branch from 9985737 to a14e95b Compare March 25, 2023 12:27

This comment has been minimized.

Sign in to view

rustbot added perf-regression and removed S-waiting-on-perf labels Mar 25, 2023

Noratrieb mentioned this pull request Mar 25, 2023

Try to use thin slices in some places in the HIR #109594

Closed

Noratrieb force-pushed the dropless-expr branch from a14e95b to 359781c Compare March 25, 2023 19:57

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the dropless-expr branch from 359781c to e616391 Compare April 16, 2023 14:57

bors added S-waiting-on-bors and removed S-waiting-on-review labels Apr 16, 2023

rust-cloud-vms bot force-pushed the dropless-expr branch from e616391 to 34ed5c3 Compare April 16, 2023 15:35

bors added the merged-by-bors label Apr 17, 2023

bors merged commit 53ac4f8 into rust-lang:master Apr 17, 2023

rustbot added this to the 1.71.0 milestone Apr 17, 2023

Noratrieb deleted the dropless-expr branch April 17, 2023 08:39

rustbot added the perf-regression label Apr 17, 2023

xFrednet mentioned this pull request Apr 17, 2023

Bump rust nightly -> 2023-04-20 rust-marker/marker#123

Closed

Alloc hir::Lit in an arena to remove the destructor from Expr #109588

Alloc hir::Lit in an arena to remove the destructor from Expr #109588

Conversation

Noratrieb commented Mar 25, 2023

rustbot commented Mar 25, 2023

Noratrieb commented Mar 25, 2023

This comment has been minimized.

bors commented Mar 25, 2023

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

bors commented Mar 25, 2023

rustbot commented Mar 25, 2023

Noratrieb commented Mar 25, 2023

This comment has been minimized.

bors commented Mar 25, 2023

bors commented Mar 25, 2023

bors commented Mar 25, 2023

This comment has been minimized.

rust-timer commented Mar 25, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Noratrieb commented Mar 25, 2023

This comment has been minimized.

Noratrieb commented Apr 16, 2023

Noratrieb commented Apr 16, 2023

bors commented Apr 16, 2023

Noratrieb commented Apr 16, 2023 • edited Loading

Noratrieb commented Apr 16, 2023

bors commented Apr 16, 2023

Noratrieb commented Apr 16, 2023

bors commented Apr 16, 2023

bors commented Apr 16, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

rust-timer commented Apr 17, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Noratrieb commented Apr 17, 2023

nnethercote commented Apr 17, 2023

Noratrieb commented Apr 17, 2023 • edited Loading

lqd commented Apr 17, 2023

rylev commented Apr 19, 2023

Noratrieb commented Apr 19, 2023

Alloc `hir::Lit` in an arena to remove the destructor from `Expr` #109588

Alloc `hir::Lit` in an arena to remove the destructor from `Expr` #109588

Noratrieb commented Apr 16, 2023 •

edited

Loading

Noratrieb commented Apr 17, 2023 •

edited

Loading