shrink OpTy back down to 80 bytes #99097

RalfJung · 2022-07-09T20:37:52Z

This shares the first commit with #99013, but seeing the perf trouble in that PR, maybe this one is worth considering first. It gets the size of OpTy down to how it was before #98846.

r? @oli-obk

rustbot · 2022-07-09T20:37:54Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

RalfJung · 2022-07-09T20:39:26Z

Let's see if this undoes the Max RSS regressions from #98846.

@bors try @rust-timer queue

rust-timer · 2022-07-09T20:39:28Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-09T20:39:34Z

⌛ Trying commit da4adcf29a2d797d416fb3aa978a23d973c37641 with merge f2f6207794ada785a91f6dbd3a9c4a29924b54ab...

bors · 2022-07-09T22:04:55Z

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

rust-timer · 2022-07-09T22:04:56Z

Queued f2f6207794ada785a91f6dbd3a9c4a29924b54ab with parent f893495, future comparison URL.

fbstj · 2022-07-09T22:06:43Z

compiler/rustc_const_eval/src/interpret/operand.rs

-            )?)?,
-            op.layout,
-        ));
+        // Sanity-check the type we ended up with.


duplicate comment?

compiler/rustc_const_eval/src/interpret/operand.rs

RalfJung · 2022-07-09T23:54:08Z

Blocked on #99101.

Kobzol · 2022-07-10T10:54:31Z

I wouldn't worry about the max RSS regression, it has a low significance factor and it's just on a single hello-world benchmark, we can safely ignore that.

…-obk interpret: refactor projection handling code Moves our projection handling code into a common file, and avoids the use of a general mplace-based fallback function by have more specialized implementations. mplace_index (and the other slice-related functions) could be more efficient by copy-pasting the body of operand_index. Or we could do some trait magic to share the code between them. But for now this is probably fine. This is the common part of rust-lang#99013 and rust-lang#99097. I am seeing some strange perf results so this probably should be its own change so we know which diff caused which perf changes... r? `@oli-obk`

bors · 2022-07-13T05:27:40Z

☔ The latest upstream changes (presumably #99101) made this pull request unmergeable. Please resolve the merge conflicts.

these are not things you *should* usually be comparing for equality...

RalfJung · 2022-07-13T14:22:29Z

All right, finally ready for review. :)

Also just to be sure...
@bors try @rust-timer queue

rust-timer · 2022-07-13T14:22:31Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-13T14:22:40Z

⌛ Trying commit b077049 with merge a0d6fe90aadd2f3d45db9497150c9f211b4adf24...

bors · 2022-07-13T15:59:42Z

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

rust-timer · 2022-07-13T15:59:43Z

Queued a0d6fe90aadd2f3d45db9497150c9f211b4adf24 with parent ca4e394, future comparison URL.

rust-timer · 2022-07-13T17:15:24Z

Finished benchmarking commit (a0d6fe90aadd2f3d45db9497150c9f211b4adf24): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.3%	3.7%	6
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.0%	3.0%	1
Improvements 🎉 (primary)	-2.9%	-2.9%	1
Improvements 🎉 (secondary)	-5.0%	-9.2%	2
All 😿🎉 (primary)	-2.9%	-2.9%	1

Cycles

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	5.6%	7.2%	6
Improvements 🎉 (primary)	-3.0%	-3.0%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-3.0%	-3.0%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

RalfJung · 2022-07-13T18:21:13Z

I guess that is not too surprising, all these layout() calls will not help with perf.

@rust-lang/wg-compiler-performance what do you think? This PR fixes a memory consumption regression (introduced by #98846) at the cost of some performance.

nnethercote · 2022-07-14T01:03:11Z

max-rss isn't affected much by this PR. Instructions only regress for ctfe-stress-5, which is a stress test and not so important.

But then, I think max-rss also wasn't affected much by #98846, so I'm not sure if this PR is really necessary?

RalfJung · 2022-07-15T12:12:15Z

But then, I think max-rss also wasn't affected much by #98846, so I'm not sure if this PR is really necessary?

Well, I saw the 6% max regression in a primary benchmark and thought that had to be fixed.

Later I learned max-RSS is very noisy, so... if you tell me the RSS numbers in #98846 are fine, I am okay with closing this PR.

nnethercote · 2022-07-16T00:14:08Z

I think the RSS numbers in #98846 are fine. For the affected benchmarks, it's only 1 or 2 of the runs, which is a clue that it's just noise. Also, the significance factors are very low, just a little over 1, which is the threshold for displaying. In clearer cases the significant factor will be over 10, sometimes over 50.

All this is unclear with deep understanding of rustc-perf, so apologies for that. We are trying to improve this, but presenting this data in a way that is both precise and comprehensible to non-experts is a genuine challenge.

RalfJung · 2022-07-16T00:48:32Z

All this is unclear with deep understanding of rustc-perf, so apologies for that. We are trying to improve this, but presenting this data in a way that is both precise and comprehensible to non-experts is a genuine challenge.

I know, and no need to apologize. :) After all, we then have you around to ask questions when things are unclear. :D

I'll close this PR then. I will investigate making OpTy not-Copy so that we avoid copying such a large type unnecessarily.

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 9, 2022

rust-highfive assigned oli-obk Jul 9, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 9, 2022

RalfJung mentioned this pull request Jul 9, 2022

interpret: get rid of MemPlaceMeta::Poison #99013

Merged

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 9, 2022

fbstj reviewed Jul 9, 2022

View reviewed changes

RalfJung force-pushed the opty-shrink branch from da4adcf to 3472cb4 Compare July 9, 2022 22:16

RalfJung commented Jul 9, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/operand.rs Outdated Show resolved Hide resolved

RalfJung force-pushed the opty-shrink branch from cf8ae0e to a5fcc5b Compare July 9, 2022 22:18

This comment was marked as outdated.

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 Jul 9, 2022

This comment was marked as outdated.

Sign in to view

RalfJung mentioned this pull request Jul 9, 2022

interpret: refactor projection handling code #99101

Merged

RalfJung added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2022

RalfJung force-pushed the opty-shrink branch from a5fcc5b to a7cf9a6 Compare July 9, 2022 23:54

RalfJung removed the perf-regression Performance regression. label Jul 12, 2022

RalfJung added 2 commits July 13, 2022 10:21

shrink OpTy back down to 80 bytes

3740152

remove some unused trait impls

b077049

these are not things you *should* usually be comparing for equality...

RalfJung force-pushed the opty-shrink branch from a7cf9a6 to b077049 Compare July 13, 2022 14:21

RalfJung added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 13, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 13, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 13, 2022

RalfJung closed this Jul 16, 2022

RalfJung deleted the opty-shrink branch July 16, 2022 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shrink OpTy back down to 80 bytes #99097

shrink OpTy back down to 80 bytes #99097

RalfJung commented Jul 9, 2022

rustbot commented Jul 9, 2022

RalfJung commented Jul 9, 2022

rust-timer commented Jul 9, 2022

bors commented Jul 9, 2022

bors commented Jul 9, 2022

rust-timer commented Jul 9, 2022

fbstj Jul 9, 2022

This comment was marked as outdated.

This comment was marked as outdated.

RalfJung commented Jul 9, 2022

Kobzol commented Jul 10, 2022

bors commented Jul 13, 2022

RalfJung commented Jul 13, 2022

rust-timer commented Jul 13, 2022

bors commented Jul 13, 2022

bors commented Jul 13, 2022

rust-timer commented Jul 13, 2022

rust-timer commented Jul 13, 2022

RalfJung commented Jul 13, 2022

nnethercote commented Jul 14, 2022

RalfJung commented Jul 15, 2022 •

edited

Loading

nnethercote commented Jul 16, 2022

RalfJung commented Jul 16, 2022

shrink OpTy back down to 80 bytes #99097

shrink OpTy back down to 80 bytes #99097

Conversation

RalfJung commented Jul 9, 2022

rustbot commented Jul 9, 2022

RalfJung commented Jul 9, 2022

rust-timer commented Jul 9, 2022

bors commented Jul 9, 2022

bors commented Jul 9, 2022

rust-timer commented Jul 9, 2022

fbstj Jul 9, 2022

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

RalfJung commented Jul 9, 2022

Kobzol commented Jul 10, 2022

bors commented Jul 13, 2022

RalfJung commented Jul 13, 2022

rust-timer commented Jul 13, 2022

bors commented Jul 13, 2022

bors commented Jul 13, 2022

rust-timer commented Jul 13, 2022

rust-timer commented Jul 13, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

RalfJung commented Jul 13, 2022

nnethercote commented Jul 14, 2022

RalfJung commented Jul 15, 2022 • edited Loading

nnethercote commented Jul 16, 2022

RalfJung commented Jul 16, 2022

RalfJung commented Jul 15, 2022 •

edited

Loading