Rollup of 7 pull requests #114331
Merged
Rollup of 7 pull requests #114331
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Backtrace doesn't have visible mutable state.
This minor change removes the need to reverse resulting digits. Since reverse is O(|digit_num|) but bounded by 128, it's unlikely to be a noticeable in practice. At the same time, this code is also a 1 line shorter, so combined with tiny perf win, why not? I ran https://gist.github.com/ttsugriy/ed14860ef597ab315d4129d5f8adb191 on M1 macbook air and got a small improvement ``` Running benches/base_n_benchmark.rs (target/release/deps/base_n_benchmark-825fe5895b5c2693) push_str/old time: [14.180 µs 14.313 µs 14.462 µs] Performance has improved. Found 5 outliers among 100 measurements (5.00%) 4 (4.00%) high mild 1 (1.00%) high severe push_str/new time: [13.741 µs 13.839 µs 13.973 µs] Performance has improved. Found 8 outliers among 100 measurements (8.00%) 3 (3.00%) high mild 5 (5.00%) high severe ```
The actual motivation here is to prevent `rustfmt` from suddenly reformatting these enum variants onto a single line, when they become slightly shorter in the future. But there's no harm in adding some helpful documentation at the same time.
Because the three kinds of operand are now distinguished explicitly, we no longer need fiddly code to disambiguate counter IDs and expression IDs based on the total number of counters/expressions in a function. This does increase the size of operands from 4 bytes to 8 bytes, but that shouldn't be a big deal since they are mostly stored inside boxed structures, and the current coverage code is not particularly size-optimized anyway.
Operand types are now tracked explicitly, so there is no need for expression IDs to avoid counter IDs by descending from `u32::MAX`. Instead they can just count up from 0, and can be used directly as indices when necessary.
Operand types are now tracked explicitly, so there is no need to reserve ID 0 for the special always-zero counter. As part of the renumbering, this change fixes an off-by-one error in the way counters were counted by the `coverageinfo` query. As a result, functions should now have exactly the number of counters they actually need, instead of always having an extra counter that is never used.
…dtolnay Implement RefUnwindSafe for Backtrace Backtrace doesn't have visible mutable state. See also https://internals.rust-lang.org/t/should-backtrace-be-refunwindsafe/17169?u=xfix
coverage: Replace `ExpressionOperandId` with enum `Operand` *This is one step in my larger coverage refactoring ambitions described at <https://github.com/rust-lang/compiler-team/issues/645>.* LLVM coverage has a concept of “mapping expressions” that allow a span's execution count to be computed as a simple arithmetic expression over other counters/expressions, instead of requiring a dedicated physical counter for every control-flow branch. These expressions have an operator (`+` or `-`) and two operands. Operands are currently represented as `ExpressionOperandId`, which wraps a `u32` with the following semantics: - 0 represents a special counter that always has a value of zero - Values ascending from 1 represent counter IDs - Values descending from `u32::MAX` represent the IDs of other expressions --- This change replaces that whole `ExpressionOperandId` scheme with a simple enum that explicitly distinguishes between the three cases. This lets us remove a lot of fiddly code for dealing with the different operand kinds: - Previously it was only possible to distinguish between counter-ID operands and expression-ID operands by comparing the operand ID with the total number of counters in a function. This is unnecessary now that the enum distinguishes them explicitly. - There's no need for expression IDs to descend from `u32::MAX` and then get translated into zero-based indices in certain places. Now that they ascend from zero, they can be used as indices directly. - There's no need to reserve ID number 0 for the special zero operand, since it can just have its own variant in the enum, so counter IDs can count up from 0. (Making counter IDs ascend from 0 also lets us fix an off-by-one error in the query for counting the total number of counters, which would cause LLVM to emit an extra unused counter for every instrumented function.) --- This PR may be easiest to review as individual patches, since that breaks it up into clearly distinct parts: - Replace a `u32` wrapper with an explicit enum, without changing the semantics of the underlying IDs being stored. - Change the numbering scheme used by `Operand::Expression` to make expression IDs ascend from 0 (instead of descending from `u32::MAX`). - Change the numbering scheme used by `Operand::Counter` to make counter IDs ascend from 0 (instead of ascending from 1).
…owLii Use parking lot's rwlock even without parallel-rustc Considering that this doesn't affect perf, I think we should use the simplest solution.
Improve diagnostic for wrong borrow on binary operations This PR improves the diagnostic for wrong borrow on binary operations by suggesting to reborrow on appropriate expressions. ```diff + = note: an implementation for `&Foo * &Foo` exist + help: consider reborrowing both sides + | + LL | let _ = &*ref_mut_foo * &*ref_mut_foo; + | ++ ++ ``` Fixes rust-lang#109352
…=oli-obk interpret: fix alignment handling for Repeat expressions
[rustc_data_structures][perf] Simplify base_n::push_str. This minor change removes the need to reverse resulting digits. Since reverse is O(|digit_num|) but bounded by 128, it's unlikely to be a noticeable in practice. At the same time, this code is also a 1 line shorter, so combined with tiny perf win, why not? I ran https://gist.github.com/ttsugriy/ed14860ef597ab315d4129d5f8adb191 on M1 macbook air and got a small improvement ``` Running benches/base_n_benchmark.rs (target/release/deps/base_n_benchmark-825fe5895b5c2693) push_str/old time: [14.180 µs 14.313 µs 14.462 µs] Performance has improved. Found 5 outliers among 100 measurements (5.00%) 4 (4.00%) high mild 1 (1.00%) high severe push_str/new time: [13.741 µs 13.839 µs 13.973 µs] Performance has improved. Found 8 outliers among 100 measurements (8.00%) 3 (3.00%) high mild 5 (5.00%) high severe ```
Cover statements for stable_mir Added missing statements to stable_mir, used opaque types for few types that are only used for diagnostic. cc rust-lang/project-stable-mir#16 r? `@oli-obk`
rustbot
added
S-waiting-on-review
|
@bors r+ rollup=never p=7 |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
📌 Perf builds for each rolled up PR:
previous master: 828bdc2c26 In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
|
Finished benchmarking commit (4896daa): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: missing data |
|
Too small of a regression to warrant investigation. Might just be noise. @rustbot label: +perf-regression-triaged |
rustbot
added
the
perf-regression-triaged
joboet
added a commit
to joboet/rust
that referenced
this pull request
Aug 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Successful merges:
ExpressionOperandIdwith enumOperand#113428 (coverage: ReplaceExpressionOperandIdwith enumOperand)r? @ghost
@rustbot modify labels: rollup
Create a similar rollup