Skip to content

Conversation

fee1-dead
Copy link
Member

An attempt to address the regression at #142240 (comment)

r? @oli-obk

cc @nnethercote who might have a better understanding of the performance implications

@rustbot 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 Jun 12, 2025
@fee1-dead fee1-dead force-pushed the push-ynxrtswtkyxw branch from 51736c3 to ac92e87 Compare June 12, 2025 06:03
@fee1-dead
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 12, 2025
@fee1-dead fee1-dead changed the title early linting: avoiding redundant calls to check_id early linting: avoid redundant calls to check_id Jun 12, 2025
bors added a commit that referenced this pull request Jun 12, 2025
early linting: avoid redundant calls to `check_id`

An attempt to address the regression at #142240 (comment)

r? `@oli-obk`

cc `@nnethercote` who might have a better understanding of the performance implications
@bors
Copy link
Collaborator

bors commented Jun 12, 2025

⌛ Trying commit ac92e87 with merge 73e31b9...

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Jun 12, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 12, 2025
@oli-obk
Copy link
Contributor

oli-obk commented Jun 12, 2025

Sad. Would need to revert #142305, too then. At this point it may be prudent to remove visit_id entirely as it's a footgun

@fee1-dead
Copy link
Member Author

it might still be possible to keep using visit_id, I just haven't looked into it deeply yet. After removing the redundant calls we can do some perf measurements. If it still is bad then we should revert to previous state.

@fee1-dead fee1-dead force-pushed the push-ynxrtswtkyxw branch from ac92e87 to ae8ca1f Compare June 12, 2025 11:01
@fee1-dead
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

bors added a commit that referenced this pull request Jun 12, 2025
early linting: avoid redundant calls to `check_id`

An attempt to address the regression at #142240 (comment)

r? `@oli-obk`

cc `@nnethercote` who might have a better understanding of the performance implications
@bors
Copy link
Collaborator

bors commented Jun 12, 2025

⌛ Trying commit ae8ca1f with merge 9fe6114...

@bors
Copy link
Collaborator

bors commented Jun 12, 2025

☀️ Try build successful - checks-actions
Build commit: 9fe6114 (9fe61147dc9031465e04140f03586e3d72f36e6c)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9fe6114): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in 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.3% [-0.6%, -0.1%] 30
Improvements ✅
(secondary)
-0.4% [-1.0%, -0.0%] 36
All ❌✅ (primary) -0.3% [-0.6%, -0.1%] 30

Max RSS (memory usage)

Results (primary 0.4%)

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

mean range count
Regressions ❌
(primary)
1.3% [1.2%, 1.4%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.5% [-1.5%, -1.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [-1.5%, 1.4%] 3

Cycles

Results (secondary -7.0%)

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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-7.0% [-7.0%, -7.0%] 1
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 755.158s -> 754.58s (-0.08%)
Artifact size: 372.13 MiB -> 372.08 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 12, 2025
@oli-obk
Copy link
Contributor

oli-obk commented Jun 12, 2025

@bors r+ yay

@bors
Copy link
Collaborator

bors commented Jun 12, 2025

📌 Commit ae8ca1f has been approved by oli-obk

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 12, 2025
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 12, 2025
@bors
Copy link
Collaborator

bors commented Jun 15, 2025

⌛ Testing commit ae8ca1f with merge 75e7cf5...

@bors
Copy link
Collaborator

bors commented Jun 15, 2025

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 75e7cf5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 15, 2025
@bors bors merged commit 75e7cf5 into rust-lang:master Jun 15, 2025
11 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 15, 2025
Copy link
Contributor

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 0cbc076 (parent) -> 75e7cf5 (this PR)

Test differences

No test diffs found

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 75e7cf5f85aad82331a38deff24845b63eaf30f3 --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. mingw-check-tidy: 93.3s -> 70.3s (-24.6%)
  2. dist-x86_64-apple: 10591.1s -> 9474.1s (-10.5%)
  3. dist-apple-various: 5925.9s -> 6480.9s (9.4%)
  4. x86_64-apple-1: 7439.8s -> 6758.9s (-9.2%)
  5. dist-aarch64-apple: 5834.0s -> 5412.1s (-7.2%)
  6. dist-android: 2656.6s -> 2470.0s (-7.0%)
  7. x86_64-msvc-1: 8798.8s -> 8284.4s (-5.8%)
  8. dist-loongarch64-linux: 6108.5s -> 5755.7s (-5.8%)
  9. x86_64-gnu-llvm-19-3: 6600.3s -> 6897.5s (4.5%)
  10. dist-arm-linux-musl: 5272.8s -> 5498.0s (4.3%)
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.

@fee1-dead fee1-dead deleted the push-ynxrtswtkyxw branch June 15, 2025 13:08
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (75e7cf5): 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.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
-0.3% [-0.6%, -0.1%] 29
Improvements ✅
(secondary)
-0.4% [-1.0%, -0.2%] 31
All ❌✅ (primary) -0.3% [-0.6%, -0.1%] 29

Max RSS (memory usage)

Results (secondary 4.6%)

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)
4.6% [4.6%, 4.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results (secondary 3.5%)

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)
3.5% [2.5%, 4.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 757.399s -> 756.494s (-0.12%)
Artifact size: 372.20 MiB -> 372.15 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants