Copy link

Contributor

nnethercote commented 21 days ago

This will be a complex one to evaluate, because there are various contradictory metrics results, e.g. some improving, some worsening. Anyway, let's get a perf run started to serve as the basis for any discussion:

@bors try @rust-timer queue

rust-timer commented 21 days ago

Awaiting bors try build completion.

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

nnethercote commented 20 days ago

@bors try

bors commented 20 days ago

Trying commit fd92286 with merge 9eec961...

bors commented 20 days ago

Try build successful - checks-actions
Build commit: 9eec961 (9eec9616e3c91ff7231a3fadff261984e83f251f)

rust-timer commented 20 days ago

rust-timer commented 20 days ago

Finished benchmarking commit (9eec961): comparison url.

Summary: This benchmark run shows 115 relevant regressions to instruction counts.

• Average relevant regression: 1.1%
• Largest regression in instruction counts: 5.5% on full builds of clap-rs check

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 led 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-review -S-waiting-on-perf +perf-regression

nnethercote commented 20 days ago •

If you look at instruction counts, it looks bad. If you look at every other metric, it looks good. One of the cases where instruction counts it misleading, I think.

A big part of this: x86-64 has "string processing" instructions like REPE MOVS. They effectively implement a loop, and things like memcpy can be implemented with them. A single string processing instruction is counted as one instruction by hardware counters, which perf uses. But Cachegrind counts them as N instructions, where N is the number of repetitions. (See section 4.1 of this paper for details.) And this commit gets rid of a lot of memcpy calls within iterate_to_fixpoint. If we went by Cachegrind's instruction counts, rather than perf's instruction counts, things would look substantially better.

nnethercote commented 20 days ago

This also improves the http crate quite a bit, though not as much as keccak. Plus I have some draft code locally that uses Rc within chunks that reduces memory usage quite a bit more, mostly on keccak.

nnethercote commented 20 days ago

BTW, this will fix #54208, if it ends up being merged.

Not sure if draft status is intended to reflect some unreadiness, but assigning myself to at least take an initial look.

nnethercote commented 20 days ago

Not sure if draft status is intended to reflect some unreadiness, but assigning myself to at least take an initial look.

Thanks! The code quality isn't great right now, there are numerous "njn:" comments indicating things that need to be fixed, and the Rc addition isn't uploaded yet. But I'm happy to hear early feedback :)

nnethercote commented 20 days ago

I have updated the code. The second commit contains the use of Rc.

@bors try @rust-timer queue

rust-timer commented 20 days ago

Awaiting bors try build completion.

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

bors commented 20 days ago

Trying commit 3ba04f6 with merge 5a26767...

bors commented 20 days ago

Try build successful - checks-actions
Build commit: 5a26767 (5a267675a45b7dcaef6afd739f55f3b207eaba3b)

rust-timer commented 20 days ago

rust-timer commented 20 days ago

Finished benchmarking commit (5a26767): comparison url.

Summary: This benchmark run shows 6 relevant improvements but 121 relevant regressions to instruction counts.

• Average relevant regression: 1.1%
• Average relevant improvement: -3.8%
• Largest improvement in instruction counts: -5.5% on full builds of keccak debug
• Largest regression in instruction counts: 6.3% on full builds of clap-rs check

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 led 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-review -S-waiting-on-perf +perf-regression

bors commented 17 days ago

Trying commit d97c79e with merge 9003e92...

bors commented 17 days ago

Try build successful - checks-actions
Build commit: 9003e92 (9003e92b6f7298995cc9772da0ec54c403a09a7e)

rust-timer commented 17 days ago

rust-timer commented 17 days ago

Finished benchmarking commit (9003e92): comparison url.

Summary: This benchmark run shows 12 relevant improvements but 105 relevant regressions to instruction counts.

• Average relevant regression: 1.0%
• Average relevant improvement: -2.8%
• Largest improvement in instruction counts: -5.4% on full builds of keccak debug
• Largest regression in instruction counts: 6.1% on full builds of clap-rs check

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 led 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-review -S-waiting-on-perf +perf-regression

nnethercote commented 17 days ago •

Finished benchmarking commit (9003e92): comparison url.

Summary: This benchmark run shows 12 relevant improvements tada but 105 relevant regressions crying_cat_face to instruction counts.

As mentioned here and here, this is a rare case where instruction counts are misleading. Instead look at wall-time and cycles (some big wins along with the usual noise) along with max-rss and faults (some massive wins along with the usual noise).

@rustbot label: +perf-regression-triaged

Mostly nits, r=me if you don't make major changes

nnethercote commented 12 days ago

@bors r=Mark-Simulacrum

bors commented 12 days ago

Commit 36b495f has been approved by Mark-Simulacrum

bors commented 12 days ago

Testing commit 36b495f with merge bafe8d0...

nnethercote commented 12 days ago

I took some extra measurements on my machine for affected benchmarks:

• From rustc-perf: cranelift-codegen, inflate, keccak, match-stress-enum, unicode_normalization
• External: http-0.2.6, rust-language-tags-0.3.2, tinyvec-1.5.1, vte-0.10.1

instructions:u

As mentioned above, these aren't useful for this change, but I've put them here for completeness.

Benchmark & Profile Scenario % Change Significance Factor? Chunk0 Chunk1

match-stress-enum check full 3.56% 17.82x 6206276362.00 6427489110.00

unicode_normalization check full 2.61% 13.05x 4394337716.00 4509070318.00

http check full -2.17% 10.85x 5639733100.00 5517400820.00

inflate check full 1.07% 5.33x 4265448539.00 4310912103.00

keccak check full -1.05% 5.23x 23069621635.00 22828169903.00

cranelift-codegen check full 0.82% 4.08x 14411424274.00 14529074725.00

vte check full -0.73% 3.67x 3260641577.00 3236722513.00

tinyvec check full 0.56% 2.82x 3378759222.00 3397785087.00

cycles

The cycle results for this PR were consistently better on CI than they were for my local builds. Maybe because my local builds don't have PGO? Anyway, these aren't all that reliable.

Benchmark & Profile Scenario % Change Significance Factor? Chunk0 Chunk1

match-stress-enum check full 5.49% 27.47x 2037211439.00 2149120206.00

vte check full -5.06% 25.31x 2192701285.00 2081692906.00

keccak check full -5.05% 25.26x 13616846225.00 12929031896.00

http check full -4.34% 21.71x 4499140807.00 4303815599.00

cranelift-codegen check full 3.07% 15.37x 12658880013.00 13047971256.00

rust-language-tags check full -2.57% 12.85x 2098872505.00 2044922945.00

unicode_normalization check full -1.91% 9.53x 2663754160.00 2613007641.00

wall-time

Same story for wall-time as for cycles.

Benchmark & Profile Scenario % Change Significance Factor? Chunk0 Chunk1

match-stress-enum check full 12.25% 61.26x 0.59 0.67

http check full -8.70% 43.51x 1.26 1.15

keccak check full -7.74% 38.69x 3.56 3.29

vte check full -7.53% 37.67x 0.72 0.67

unicode_normalization check full 4.79% 23.95x 0.84 0.88

inflate check full 1.64% 8.20x 0.63 0.64

cranelift-codegen check full 1.16% 5.80x 3.28 3.31

rust-language-tags check full -0.99% 4.95x 0.68 0.68

tinyvec check full -0.95% 4.75x 0.62 0.62

max-rss

Huge wins here.

Benchmark & Profile Scenario % Change Significance Factor? Chunk0 Chunk1

keccak check full -59.05% 295.25x 974100.00 398892.00

http check full -32.11% 160.54x 390080.00 264832.00

vte check full -26.96% 134.78x 256144.00 187096.00

unicode_normalization check full -15.45% 77.27x 264228.00 223396.00

match-stress-enum check full -11.79% 58.94x 162364.00 143224.00

inflate check full -9.10% 45.49x 205048.00 186392.00

rust-language-tags check full -8.39% 41.93x 229596.00 210344.00

tinyvec check full -7.16% 35.79x 163636.00 151924.00

cranelift-codegen check full -4.03% 20.14x 400552.00 384420.00

faults

Huge wins here, too.

Benchmark & Profile Scenario % Change Significance Factor? Chunk0 Chunk1

keccak check full -60.21% 301.06x 240390.00 95647.00

http check full -46.54% 232.69x 68926.00 36849.00

vte check full -46.42% 232.08x 43003.00 23043.00

unicode_normalization check full -27.03% 135.17x 39006.00 28461.00

match-stress-enum check full -24.91% 124.55x 19325.00 14511.00

inflate check full -21.48% 107.39x 23373.00 18353.00

tinyvec check full -18.21% 91.07x 18809.00 15383.00

rust-language-tags check full -15.42% 77.08x 33191.00 28074.00

cranelift-codegen check full -5.83% 29.13x 75575.00 71172.00

bors commented 12 days ago

Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing bafe8d0 to master...

rust-timer commented 12 days ago

Finished benchmarking commit (bafe8d0): comparison url.

Summary: This benchmark run shows 6 relevant improvements but 107 relevant regressions to instruction counts.

• Average relevant regression: 1.0%
• Average relevant improvement: -3.8%
• Largest improvement in instruction counts: -5.3% on full builds of keccak debug
• Largest regression in instruction counts: 6.0% on full builds of clap-rs check

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

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

nnethercote commented 12 days ago

As mentioned here and here, this is a rare case where instruction counts are misleading. Instead look at wall-time and cycles (some big wins along with the usual noise) along with max-rss and faults (some massive wins along with the usual noise).

@rustbot label: +perf-regression-triaged