Similar to the last commit, it's more of a `Parser`-level concern than a
`TokenCursor`-level concern. And the struct size reductions are nice.
After this change, `TokenCursor` is as minimal as possible (two fields
and two methods) which is nice.
It's more of a `Parser`-level concern than a `TokenCursor`-level
concern. Also, `num_bump_calls` is a more accurate name, because it's
incremented in `Parser::bump`.
This code currently uses a `while` loop and gathers token trees into a
vector, but it only succeeds in the case where there is a single token
tree. We can instead just call `parse_token_tree` once and then look for
`Eof` to detect the success case.
Comparing vectors of string parts yields the same result but avoids
unnecessary `join` and potential allocation for resulting `String`.
This code is cold so it's unlikely to have any measurable impact, but
considering but since it's also simpler, why not? :)
Filter out short-lived LLVM diagnostics before they reach the rustc handler
During profiling I saw remark passes being unconditionally enabled: for example `Machine Optimization Remark Emitter`.
The diagnostic remarks enabled by default are [from missed optimizations and opt analyses](https://github.com/rust-lang/rust/pull/113339#discussion_r1259480303). They are created by LLVM, passed to the diagnostic handler on the C++ side, emitted to rust, where they are unpacked, C++ strings are converted to rust, etc.
Then they are discarded in the vast majority of the time (i.e. unless some kind of `-Cremark` has enabled some of these passes' output to be printed).
These unneeded allocations are very short-lived, basically only lasting between the LLVM pass emitting them and the rust handler where they are discarded. So it doesn't hugely impact max-rss, and is only a slight reduction in instruction count (cachegrind reports a reduction between 0.3% and 0.5%) _on linux_. It's possible that targets without `jemalloc` or with a worse allocator, may optimize these less.
It is however significant in the aggregate, looking at the total number of allocated bytes:
- it's the biggest source of allocations according to dhat, on the benchmarks I've tried e.g. `syn` or `cargo`
- allocations on `syn` are reduced by 440MB, 17% (from 2440722647 bytes total, to 2030461328 bytes)
- allocations on `cargo` are reduced by 6.6GB, 19% (from 35371886402 bytes total, to 28723987743 bytes)
Some of these diagnostics objects [are allocated in LLVM](https://github.com/rust-lang/rust/pull/113339#discussion_r1252387484) *before* they're emitted to our diagnostic handler, where they'll be filtered out. So we could remove those in the future, but that will require changing a few LLVM call-sites upstream, so I left a FIXME.
Move doc comment desugaring out of `TokenCursor`.
It's awkward that `TokenCursor` sometimes desugars doc comments on the fly, but usually doesn't.
r? `@petrochenkov`
now that remarks are filtered before cg_llvm's diagnostic handler callback
is called, we don't need to do the filtering post c++-to-rust conversion
of the diagnostic.
this will eliminate many short-lived allocations (e.g. 20% of the memory used
building cargo) when unpacking the diagnostic and converting its various
C++ strings into rust strings, just to be filtered out most of the time.
cleanup: remove pointee types
This can't be merged until the oldest LLVM version we support uses opaque pointers, which will be the case after #114148. (Also note `-Cllvm-args="-opaque-pointers=0"` can technically be used in LLVM 15, though I don't think we should support that configuration.)
I initially hoped this would provide some minor perf win, but in https://github.com/rust-lang/rust/pull/105412#issuecomment-1341224450 it had very little impact, so this is only valuable as a cleanup.
As a followup, this will enable #96242 to be resolved.
r? `@ghost`
`@rustbot` label S-blocked
[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
```
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 https://github.com/rust-lang/rust/issues/109352
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).
Change default panic handler message format.
This changes the default panic hook's message format from:
```
thread '{thread}' panicked at '{message}', {location}
```
to
```
thread '{thread}' panicked at {location}:
{message}
```
This puts the message on its own line without surrounding quotes, making it easiser to read. For example:
Before:
```
thread 'main' panicked at 'env variable `IMPORTANT_PATH` should be set by `wrapper_script.sh`', src/main.rs:4:6
```
After:
```
thread 'main' panicked at src/main.rs:4:6:
env variable `IMPORTANT_PATH` should be set by `wrapper_script.sh`
```
---
See this PR by `@nyurik,` which does that for only multi-line messages (specifically because of `assert_eq`): https://github.com/rust-lang/rust/pull/111071
This is the change that does that for *all* panic messages.
