perf: reuse green-marking's edge walk when promoting a node

[xmakro]

View all comments

[saethlin]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 2132d40 (2132d40c0e743607ddcdb42e9fba0ffb4b57eb2b, parent: d2f24127d97d8c795d1cf5fbf6037f9885f34d39)

[rust-timer]

Finished benchmarking commit (2132d40): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.8%, -0.2%]	59
Improvements ✅ (secondary)	-0.4%	[-1.0%, -0.2%]	35
All ❌✅ (primary)	-0.4%	[-0.8%, -0.2%]	59

Max RSS (memory usage)

Results (secondary 2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 2.9%, secondary 3.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.0%, 4.1%]	20
Regressions ❌ (secondary)	3.2%	[2.1%, 4.8%]	15
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.9%	[2.0%, 4.1%]	20

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 518.142s -> 518.408s (0.05%)
Artifact size: 400.91 MiB -> 400.77 MiB (-0.03%)

[JonathanBrouwer]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: a78575f (a78575f6a874926ab011cc2581dd4ac9a0dec299, parent: 4e391cf2425cf96521af17ff460e9f220e9bca00)

[rust-timer]

Finished benchmarking commit (a78575f): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.8%	[-5.1%, -0.3%]	109
Improvements ✅ (secondary)	-1.7%	[-4.4%, -0.3%]	59
All ❌✅ (primary)	-1.8%	[-5.1%, -0.3%]	109

Max RSS (memory usage)

Results (primary -1.6%, secondary -2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.8%	[-5.8%, -5.8%]	1
Improvements ✅ (secondary)	-2.5%	[-3.0%, -2.0%]	2
All ❌✅ (primary)	-1.6%	[-5.8%, 2.6%]	2

Cycles

Results (secondary 2.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 516.925s -> 517.827s (0.17%)
Artifact size: 400.84 MiB -> 400.84 MiB (0.00%)

[xmakro]

r? @cjgillot

[cjgillot]

Wow great results!

View changes since this review

[cjgillot]

@bors r+

[rust-bors]

📌 Commit bd268e1 has been approved by cjgillot

It is now in the queue for this repository.

[rust-bors]

💔 Test for e5e5700 failed: CI. Failed job:

• auto - i686-gnu-2 (web logs, enhanced plaintext logs)

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

  IMAGE: i686-gnu
  TOOLSTATE_PUBLISH: 1
##[endgroup]
    Updating crates.io index
error: failed to get `askama_derive` as a dependency of package `askama_macros v0.16.0`
    ... which satisfies dependency `askama_macros = "=0.16.0"` of package `askama v0.16.0`
    ... which satisfies dependency `askama = "^0.16.0"` of package `citool v0.1.0 (/home/runner/work/rust/rust/src/ci/citool)`

Caused by:
  failed to load source for dependency `askama_derive`

Caused by:
  unable to update registry `crates-io`

Caused by:
  download of as/ka/askama_derive failed

Caused by:
  curl failed

Caused by:

[xmakro]

Thanks for the review!

[JonathanBrouwer]

@bors retry

[rust-bors]

☀️ Test successful - CI
Approved by: cjgillot
Duration: 3h 16m 57s
Pushing b5d46ec to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 3daae5e (parent) -> b5d46ec (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard b5d46ecb51c3e4134b82570cfe718f093daa6390 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. x86_64-gnu-gcc-core-tests: 8m 2s -> 14m 28s (+80.3%)
2. dist-sparcv9-solaris: 1h 12m -> 1h 38m (+35.9%)
3. dist-i686-msvc: 2h 20m -> 1h 46m (-23.6%)
4. dist-x86_64-apple: 1h 45m -> 2h 10m (+22.8%)
5. dist-x86_64-mingw: 2h 4m -> 2h 30m (+20.9%)
6. dist-aarch64-apple: 1h 40m -> 2h 1m (+20.7%)
7. x86_64-gnu-pre-stabilization: 30m 21s -> 36m 19s (+19.7%)
8. i686-gnu-1: 1h 16m -> 1h 30m (+18.4%)
9. test-various: 1h 55m -> 2h 16m (+18.1%)
10. pr-check-1: 27m 39s -> 32m 20s (+17.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (b5d46ec): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.8%	[-5.1%, -0.2%]	114
Improvements ✅ (secondary)	-1.7%	[-4.4%, -0.5%]	58
All ❌✅ (primary)	-1.8%	[-5.1%, -0.2%]	114

Max RSS (memory usage)

Results (primary -0.9%, secondary 2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.2%	[2.5%, 3.9%]	2
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-3.7%	[-6.5%, -0.8%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-6.5%, 3.9%]	5

Cycles

Results (primary -1.3%, secondary 2.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.9%, 2.9%]	1
Improvements ✅ (primary)	-1.3%	[-1.3%, -1.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.3%	[-1.3%, -1.3%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 519.503s -> 522.814s (0.64%)
Artifact size: 401.51 MiB -> 400.99 MiB (-0.13%)

[nnethercote]

@xmakro: great results! How did you find this improvement?

[xmakro]

@nnethercote: thanks! I found this while working on #157319. The edges being color-mapped twice in quick succession in serialized.rs#L512-L532 stood out first. Storing them in a reused vec gets half the improvement. Only later I realized that try_mark_previous_green already walks the edges and both color-mappings in encode_promoted can be avoided.

[Zoxc]

Wall time is negative here, so is this actually an improvement?