Shrink StatementKind

[nnethercote]

StatementKind occurs in significant amounts in Massif profiles.

[rust-highfive]

r? @matthewjasper

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

[nnethercote]

Some max-rss results:

ucd-check
avg: -2.0% min: -4.3% max: 1.4%
style-servo-check
avg: -0.7% min: -2.6% max: 0.1%
webrender-check
avg: -0.9% min: -2.6% max: -0.0%
cargo-check
avg: -0.9% min: -2.3% max: -0.1%
piston-image-check
avg: -0.7% min: -1.8% max: 0.0%
html5ever-check
avg: -0.4% min: -1.8% max: 0.9%
regex-check
avg: -0.4% min: -1.7% max: -0.0%
tuple-stress-check
avg: 1.1% min: -0.8% max: 1.7%
sentry-cli-check
avg: -0.6% min: -1.6% max: -0.1%
helloworld-check
avg: -0.4% min: -1.6% max: 0.3%
serde-check
avg: -0.8% min: -1.6% max: -0.2%
ripgrep-check
avg: -0.7% min: -1.4% max: -0.1%
rust-unic-check
avg: 0.1% min: -0.8% max: 1.3%
unify-linearly-check
avg: -0.5% min: -1.3% max: 0.1%
crates.io-check
avg: -0.6% min: -1.3% max: -0.2%
clap-rs-check
avg: -0.5% min: -1.2% max: -0.0%
coercions-check
avg: -0.3%? min: -1.1%? max: 0.2%? ctfe-stress-check
avg: -0.5%? min: -1.1%? max: -0.2%?
futures-check
avg: -0.4% min: -1.0% max: 0.0%

The instruction counts are mostly unchanged, but I will do a rust-timer run just to double check that.

[nnethercote]

@bors try

[bors]

⌛ Trying commit e221b24 with merge c4a65be...

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[nagisa]

@nnethercote since you’re there, could you try making Vec in StatementKind::Validate a Box<[]> as well? In general nothing in MIR needs to be mutable as far as I can tell (since it is not usually mutable in place).

[nagisa]

r? @nagisa

[nnethercote]

@rust-timer build c4a65be

[rust-timer]

Success: Queued c4a65be with parent 5ad5aca, comparison URL.

[nnethercote]

could you try making Vec in StatementKind::Validate a Box<[]> as well?

It doesn't feel to me like a useful change, because it won't affect the size of StatementKind.

[nnethercote]

Hmm, it's about a 1% instruction count regression for ucd, tuple-stress and keccak.

For max-rss (and faults) it's a bigger win, with lots of results in the 1--5% range. Also, NLL seems to benefit more than non-NLL, but only the check builds run with NLL enabled, so that means the max-rss improvements are arguably under-weighted.

For other metrics there's too much noise to tell much.

Based on the above, I'm leaning towards landing this, but it's not a slam dunk and I'm happy to defer to the reviewer's decision.

[nagisa]

about a 1% instruction count regression

Are there any wall-clock time/cycle count comparisons as well? Instruction count comparisons, in my experience, are not great. Even if two distinct workloads do not differ by much, as depending on instruction order and instructions themselves, modern CPUs can easily retire wildly different number of instructions in same span of time. At the high end, I’ve seen a factor of 4 difference in a real-world work-load in an application doing the same job in same amount of time.

it won't affect the size of StatementKind

Oh, I didn’t intend to make an impression that it would beneficial for performance. I meant it would be nice-to-have purely for consistency reasons.

@bors r+

[bors]

📌 Commit e221b24 has been approved by nagisa

[nnethercote]

As per #54526 (comment), the other metrics (which includes wall-time, cycles:u, task-clock, and cpu-clock) are too noisy in this case to be useful, unfortunately. You are right that instructions is not the ideal metric, but it's the default because it's far more stable than time-related ones.

[bors]

⌛ Testing commit e221b24 with merge 4415195...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 4415195 to master...