Skip to content

Rollup of 4 pull requests #140708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 6, 2025
Merged

Conversation

GuillaumeGomez
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

Zalathar and others added 9 commits May 6, 2025 14:35
This also removes some manipulation of the function signature span that only
made sense in the context of merging non-adjacent spans.
Because we no longer merge non-adjacent spans, there is no need to use buckets
to prevent merging across hole spans.
Update examples to no longer avoid iterating arrays for rust-lang#84513
Update iterator.rs to use arrays by value

Update examples to no longer avoid iterating arrays for rust-lang#84513
coverage: Only merge adjacent coverage spans

For a long time, coverage instrumentation has automatically “merged” spans with the same control-flow into a smaller number of larger spans, even when the spans being merged are not overlapping or adjacent. This causes any source text between the original spans to be included in the merged span, which is then associated with an execution count when shown in coverage reports.

That approach causes a number of problems:
- The intervening source text can contain all sorts of things that shouldn't really be marked as executable code (e.g. nested items, parts of macro invocations, long comments). In some cases we have complicated workarounds (e.g. bucketing to avoid merging spans across nested items), but in other cases there isn't much we can do.
- Merging can have aesthetically weird effects, such as including unbalanced parentheses, because the merging process doesn't really understand what it's doing at a source code level.
- It generally leads to an accumulation of piled-on heuristics and special cases that give decent-looking results, but are fiendishly difficult to modify or replace.

Therefore, this PR aims to abolish the merging of non-adjacent coverage spans.

The big tradeoff here is that the resulting coverage metadata (embedded in the instrumented binary) tends to become larger, because the overall number of distinct spans has increased. That's unfortunate, but I see it as the inevitable cost of cleaning up the messes and inaccuracies that were caused by the old approach. And the resulting spans do tend to be more accurate to the program's actual control-flow.

---

The `.coverage` snapshot changes give an indication of how this PR will affect user-visible coverage reports. In many cases the changes to reporting are minor or even nonexistent, despite substantial changes to the metadata (as indicated by `.cov-map` snapshots).

---

try-job: aarch64-gnu
Rename `graph::implementation::Graph` to `LinkedGraph`

One of the more confusing parts of the `rustc_data_structures::graph` module is this mysteriously-named “Graph” type, which turns out to be an older standalone graph implementation that predates the traits used by the rest of the graph module.

This graph type is still used in a couple of places (for reporting certain lifetime errors, and by certain debugging/test-only checks of the query dependency graph), but hasn't had much attention in years.

This PR renames that old graph type from `implementation::Graph` to `linked_graph::LinkedGraph` to give it a more distinct identity (and make existing uses easier to find), and adds some notes to gently discourage any further use in new code.

No functional change.
…oieni

Handle PR not found in post-merge workflow

Should hopefully fix errors like [these](rust-lang#140561 (comment)).

r? `@marcoieni`
@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels May 6, 2025
@GuillaumeGomez
Copy link
Member Author

@bors r+ p=4 rollup=never

@bors
Copy link
Collaborator

bors commented May 6, 2025

📌 Commit ee0d68f has been approved by GuillaumeGomez

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2025
@bors
Copy link
Collaborator

bors commented May 6, 2025

⌛ Testing commit ee0d68f with merge 27d6200...

@bors
Copy link
Collaborator

bors commented May 6, 2025

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 27d6200 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 6, 2025
@bors bors merged commit 27d6200 into rust-lang:master May 6, 2025
7 checks passed
@rustbot rustbot added this to the 1.88.0 milestone May 6, 2025
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#136183 Update iterator.rs to use arrays by value 15c4abbac7bbf1da7a77bb20b231651d3b1626e0 (link)
#139966 coverage: Only merge adjacent coverage spans 13c5fb6417b7d2924ffe9a76239b3167af683640 (link)
#140692 Rename graph::implementation::Graph to LinkedGraph 24b5f750972d4990497cb92ada293cb8a4d8f6e4 (link)
#140703 Handle PR not found in post-merge workflow ee0f5d1b64bc17ecb6852a659142b741e7324c97 (link)

previous master: 1a95cc6f9d

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

Copy link

github-actions bot commented May 6, 2025

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 1a95cc6 (parent) -> 27d6200 (this PR)

Test differences

Show 452 test diffs

Stage 1

  • graph::implementation::tests::each_adjacent_from_a: pass -> [missing] (J0)
  • graph::implementation::tests::each_adjacent_from_b: pass -> [missing] (J0)
  • graph::implementation::tests::each_adjacent_from_c: pass -> [missing] (J0)
  • graph::implementation::tests::each_adjacent_from_d: pass -> [missing] (J0)
  • graph::implementation::tests::each_edge: pass -> [missing] (J0)
  • graph::implementation::tests::each_node: pass -> [missing] (J0)
  • graph::linked_graph::tests::each_adjacent_from_a: [missing] -> pass (J0)
  • graph::linked_graph::tests::each_adjacent_from_b: [missing] -> pass (J0)
  • graph::linked_graph::tests::each_adjacent_from_c: [missing] -> pass (J0)
  • graph::linked_graph::tests::each_adjacent_from_d: [missing] -> pass (J0)
  • graph::linked_graph::tests::each_edge: [missing] -> pass (J0)
  • graph::linked_graph::tests::each_node: [missing] -> pass (J0)

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

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 27d6200a70601f6fcf419bf2f9e37989f3624ca4 --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. dist-aarch64-linux: 5474.7s -> 7872.3s (43.8%)
  2. x86_64-apple-2: 7022.4s -> 4214.6s (-40.0%)
  3. dist-x86_64-apple: 9352.0s -> 11285.0s (20.7%)
  4. aarch64-apple: 4862.6s -> 4121.3s (-15.2%)
  5. dist-apple-various: 7609.4s -> 6461.1s (-15.1%)
  6. dist-i686-msvc: 7888.6s -> 6945.6s (-12.0%)
  7. dist-aarch64-apple: 5964.7s -> 5261.4s (-11.8%)
  8. x86_64-msvc-2: 7253.8s -> 6614.2s (-8.8%)
  9. test-various: 4179.3s -> 4487.0s (7.4%)
  10. dist-arm-linux: 4797.5s -> 5139.8s (7.1%)
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
Copy link
Collaborator

Finished benchmarking commit (27d6200): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

Results (primary 0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.3% [0.4%, 2.3%] 6
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.1% [-1.1%, -1.1%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.9% [-1.1%, 2.3%] 7

Cycles

Results (primary -0.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.6% [-0.7%, -0.4%] 9
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-0.7%, -0.4%] 9

Binary size

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

Bootstrap: 769.964s -> 769.287s (-0.09%)
Artifact size: 365.50 MiB -> 365.44 MiB (-0.02%)

@rustbot rustbot added the perf-regression Performance regression. label May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. rollup A PR which is a rollup 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants