Bypass the varint path when encoding InitMask #110343

saethlin · 2023-04-14T23:56:31Z

The data in a InitMask is stored as u64 but it is a large bitmask (not numbers) so varint encoding doesn't make sense.

saethlin · 2023-04-14T23:56:54Z

@bors try @rust-timer queue

bors · 2023-04-14T23:57:03Z

⌛ Trying commit 610a9152e521fc1239e47d6b535f29a8f41f826d with merge 07959ce106e5ec22c1d30c15040752c0790e3587...

bors · 2023-04-15T01:40:46Z

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

rust-timer · 2023-04-15T21:37:03Z

Finished benchmarking commit (07959ce106e5ec22c1d30c15040752c0790e3587): 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
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	2
Improvements ✅ (primary)	-0.6%	[-0.8%, -0.3%]	4
Improvements ✅ (secondary)	-0.4%	[-0.7%, -0.2%]	9
All ❌✅ (primary)	-0.6%	[-0.8%, -0.3%]	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)	-	-	0
Regressions ❌ (secondary)	1.5%	[1.5%, 1.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

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

rustbot · 2023-04-15T22:11:06Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

saethlin · 2023-04-15T22:13:27Z

r? @lqd

compiler/rustc_middle/src/mir/interpret/allocation/init_mask.rs

lqd

Awesome.

~~I now wonder whether the const allocations' bytes themselves really need to be LEB128-encoded as well, and what would the time and size cost/gains be if we didn't.~~ (But probably they already aren't, they shouldn't and to be clear I haven't checked)

r=me with the typo suggestion squashed

compiler/rustc_middle/src/mir/interpret/allocation/init_mask.rs

lqd · 2023-04-17T08:34:38Z

compiler/rustc_middle/src/mir/interpret/allocation/init_mask.rs

+    fn encode(&self, encoder: &mut E) {
+        encoder.emit_usize(self.blocks.len());
+        for block in &self.blocks {
+            encoder.emit_raw_bytes(&block.to_le_bytes());


Just wanted to note: I'm not familiar with the endianness rules for encoding/decoding, I had idly wondered before if we should prefer native endianness in incr data, but I think we use LE everywhere so 👍 .

lqd · 2023-04-17T17:58:10Z

@bors r+

bors · 2023-04-17T17:58:13Z

📌 Commit 69279c0 has been approved by lqd

It is now in the queue for this repository.

bors · 2023-04-17T19:31:47Z

⌛ Testing commit 69279c0 with merge 56e28e9...

bors · 2023-04-17T21:41:08Z

☀️ Test successful - checks-actions
Approved by: lqd
Pushing 56e28e9 to master...

rust-timer · 2023-04-17T22:57:05Z

Finished benchmarking commit (56e28e9): comparison URL.

Overall result: ❌ regressions - 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.1%	[0.2%, 2.9%]	11
Regressions ❌ (secondary)	3.7%	[0.2%, 8.1%]	11
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[0.2%, 2.9%]	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
Regressions ❌ (secondary)	2.3%	[1.6%, 3.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
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)	1.3%	[1.3%, 1.3%]	1
Regressions ❌ (secondary)	3.9%	[3.2%, 4.4%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.3%	[1.3%, 1.3%]	1

saethlin · 2023-04-17T23:09:29Z

Largest regressions are in cranelift-codegen and keccak. Cachegrind diff for keccak looks like this (which should not surprise anyone):

1,374,605,175  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir             file:function
--------------------------------------------------------------------------------
1,374,460,232  ???:<rustc_data_structures::obligation_forest::ObligationForest<rustc_trait_selection::traits::fulfill::PendingPredicateObligation>>::process_obligations::<rustc_trait_selection::traits::fulfill::FulfillProcessor>

This PR doesn't modify any of the related code. This looks like more optimization instability to me.

nnethercote · 2023-04-18T03:22:52Z

keccak and cranelift-codegen have just become bimodal again, with keccak giving an incredible +/- 8% swings. The other regressions (most notably diesel) are likely unrelated to that, and might be genuinely related to this PR.

nnethercote · 2023-04-18T03:25:44Z

On second thought, I just saw this, so perhaps the diesel changes are just noise.

saethlin · 2023-04-18T03:35:43Z

The diesel diff looks like this:


199,979,359  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir             file:function
--------------------------------------------------------------------------------
  435,413,890  ???:<&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_type_ir::fold::TypeFoldable<rustc_middle::ty::context::TyCtxt>>::try_fold_with::<rustc_infer::infer::resolve::OpportunisticVarResolver>
 -263,175,486  ???:<rustc_infer::traits::Obligation<rustc_middle::ty::Predicate> as rustc_type_ir::fold::TypeFoldable<rustc_middle::ty::context::TyCtxt>>::fold_with::<rustc_infer::infer::resolve::OpportunisticVarResolver>

Which also seems pretty far away from what this PR is doing. So yeah, I agree.

rylev · 2023-04-19T08:01:08Z

Marking this as triaged since this was identified as most likely being noise.

@rustbot label: +perf-regression-triaged

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 Apr 14, 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 Apr 14, 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 Apr 15, 2023

saethlin marked this pull request as ready for review April 15, 2023 22:11

rustbot assigned lqd Apr 15, 2023

nnethercote reviewed Apr 16, 2023

View reviewed changes

compiler/rustc_middle/src/mir/interpret/allocation/init_mask.rs Show resolved Hide resolved

saethlin force-pushed the encode-initmask branch 2 times, most recently from 79467e3 to ada897b Compare April 17, 2023 01:04

lqd approved these changes Apr 17, 2023

View reviewed changes

Bypass the varint path when encoding InitMask

69279c0

saethlin force-pushed the encode-initmask branch from ada897b to 69279c0 Compare April 17, 2023 13:44

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 Apr 17, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 17, 2023

bors merged commit 56e28e9 into rust-lang:master Apr 17, 2023

rustbot added this to the 1.71.0 milestone Apr 17, 2023

saethlin deleted the encode-initmask branch April 17, 2023 21:54

saethlin mentioned this pull request Apr 18, 2023

Encode hashes as bytes, not varint #110083

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bypass the varint path when encoding InitMask #110343

Bypass the varint path when encoding InitMask #110343

saethlin commented Apr 14, 2023 •

edited

Loading

saethlin commented Apr 14, 2023

This comment has been minimized.

bors commented Apr 14, 2023

bors commented Apr 15, 2023

This comment has been minimized.

rust-timer commented Apr 15, 2023

rustbot commented Apr 15, 2023

saethlin commented Apr 15, 2023

lqd left a comment •

edited

Loading

lqd Apr 17, 2023

lqd commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

rust-timer commented Apr 17, 2023

saethlin commented Apr 17, 2023

nnethercote commented Apr 18, 2023

nnethercote commented Apr 18, 2023

saethlin commented Apr 18, 2023

rylev commented Apr 19, 2023

Bypass the varint path when encoding InitMask #110343

Bypass the varint path when encoding InitMask #110343

Conversation

saethlin commented Apr 14, 2023 • edited Loading

saethlin commented Apr 14, 2023

This comment has been minimized.

bors commented Apr 14, 2023

bors commented Apr 15, 2023

This comment has been minimized.

rust-timer commented Apr 15, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

rustbot commented Apr 15, 2023

saethlin commented Apr 15, 2023

lqd left a comment • edited Loading

Choose a reason for hiding this comment

lqd Apr 17, 2023

Choose a reason for hiding this comment

lqd commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

rust-timer commented Apr 17, 2023

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

saethlin commented Apr 17, 2023

nnethercote commented Apr 18, 2023

nnethercote commented Apr 18, 2023

saethlin commented Apr 18, 2023

rylev commented Apr 19, 2023

saethlin commented Apr 14, 2023 •

edited

Loading

lqd left a comment •

edited

Loading