rust-lang
Add option to `mir::MutVisitor` to not invalidate CFG.
This also applies that option to some uses of the visitor. I had considered a design more similar to #100087 in which we detect if the CFG needs to be invalidated, but that is more difficult with the visitor API and so I decided against it. Another alternative to this design is to offer an API for "saving" and "restoring" CFG caches across arbitrary code. Such an API is more general, and so we may eventually want it anyway, but it seems overkill for this use case.
r? @tmiasko
The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions := True
configure: dist.missing-tools := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure:
configure: run `python /checkout/x.py --help`
Attempting with retry: make prepare
---
######## 11.4%
######################################################################## 100.0%
extracting /checkout/obj/build/cache/2022-07-21/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz to /checkout/obj/build/x86_64-unknown-linux-gnu/stage0
skip untracked path cpu-usage.csv during rustfmt invocations
Diff in /checkout/compiler/rustc_middle/src/mir/visit.rs at line 992:
macro_rules! basic_blocks {
macro_rules! basic_blocks {
- ($body:ident, mut) => (
+ ($body:ident, mut) => {
if Self::AVOID_INVALIDATING_CFG {
$body.basic_blocks.as_mut_preserves_cfg()
} else {
Diff in /checkout/compiler/rustc_middle/src/mir/visit.rs at line 999:
$body.basic_blocks.as_mut()
- );
- );
- ($body:ident,) => ($body.basic_blocks());
+ };
+ ($body:ident,) => {
+ $body.basic_blocks()
}
macro_rules! basic_blocks_iter {
macro_rules! basic_blocks_iter {
Diff in /checkout/compiler/rustc_middle/src/mir/visit.rs at line 1006:
- ($body:ident, mut) => (
+ ($body:ident, mut) => {
basic_blocks!($body, mut).iter_enumerated_mut()
- );
- ($body:ident,) => (
+ };
+ ($body:ident,) => {
basic_blocks!($body,).iter_enumerated()
+ };
}
macro_rules! visit_place_fns {
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2021" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_middle/src/mir/mono.rs" "/checkout/compiler/rustc_middle/src/mir/visit.rs" "/checkout/compiler/rustc_middle/src/mir/terminator.rs" "/checkout/compiler/rustc_middle/src/mir/type_visitable.rs" "/checkout/compiler/rustc_middle/src/mir/tcx.rs" "/checkout/compiler/rustc_middle/src/mir/coverage.rs" "/checkout/compiler/rustc_middle/src/mir/spanview.rs" "/checkout/compiler/rustc_middle/src/mir/switch_sources.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
thread '<unnamed>' panicked at 'tx.send(entry.into_path()) failed with sending on a closed channel', format.rs:166:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at 'tx.send(entry.into_path()) failed with sending on a closed channel', format.rs:166:17
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Any { .. }', /cargo/registry/src/github.com-1ecc6299db9ec823/ignore-0.4.18/src/walk.rs:1302:31
At some point it might be worth adding a validation of cached information behind -Zvalidate-mir.
Looks good, thanks!
@bors delegate+
At some point it might be worth adding a validation of cached information behind
-Zvalidate-mir.
Yes, definitely. At some point I want to add stateful analyses in the pass manager, at which point we'll definitely need this, so I'm kind of putting it off until then.
:pushpin: Commit 7547084ff673dcfdb2c6bcf7dc7a81190513ed40 has been approved by tmiasko
It is now in the queue for this repository.
:hourglass: Testing commit 7547084ff673dcfdb2c6bcf7dc7a81190513ed40 with merge cc4dd6fc9f1a5c798df269933c7e442b79661a86...
:sunny: Test successful - checks-actions Approved by: tmiasko Pushing cc4dd6fc9f1a5c798df269933c7e442b79661a86 to master...
Finished benchmarking commit (cc4dd6fc9f1a5c798df269933c7e442b79661a86): comparison url.
Instruction count
- Primary benchmarks: ❌ relevant regressions found
- Secondary benchmarks: mixed results
| mean[^1] | max | count[^2] | |
|---|---|---|---|
| Regressions ❌ (primary) |
0.3% | 0.4% | 3 |
| Regressions ❌ (secondary) |
1.3% | 1.5% | 6 |
| Improvements ✅ (primary) |
N/A | N/A | 0 |
| Improvements ✅ (secondary) |
-1.5% | -1.5% | 1 |
| All ❌✅ (primary) | 0.3% | 0.4% | 3 |
Max RSS (memory usage)
Results
- Primary benchmarks: no relevant changes found
- Secondary benchmarks: ❌ relevant regression found
| mean[^1] | max | count[^2] | |
|---|---|---|---|
| Regressions ❌ (primary) |
N/A | N/A | 0 |
| Regressions ❌ (secondary) |
3.1% | 3.1% | 1 |
| Improvements ✅ (primary) |
N/A | N/A | 0 |
| Improvements ✅ (secondary) |
N/A | N/A | 0 |
| All ❌✅ (primary) | N/A | N/A | 0 |
Cycles
Results
- Primary benchmarks: ✅ relevant improvement found
- Secondary benchmarks: no relevant changes found
| mean[^1] | max | count[^2] | |
|---|---|---|---|
| Regressions ❌ (primary) |
N/A | N/A | 0 |
| Regressions ❌ (secondary) |
N/A | N/A | 0 |
| Improvements ✅ (primary) |
-4.5% | -4.5% | 1 |
| Improvements ✅ (secondary) |
N/A | N/A | 0 |
| All ❌✅ (primary) | -4.5% | -4.5% | 1 |
[^1]: the arithmetic mean of the percent change [^2]: number of relevant changes
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.
@rustbot label: +perf-regression cc @rust-lang/wg-compiler-performance
This is a small enough regression that I don't think we need to investigate it deeply. I ran cachgrind diff, and I didn't see anything that really stood out to me although I can't say I understand this part of the code base deeply.
7,877,684 ???:<rustc_middle::ty::fast_reject::DeepRejectCtxt>::types_may_unify
3,047,609 ???:<rustc_middle::ty::subst::SubstFolder as rustc_middle::ty::fold::TypeFolder>::fold_ty
-2,262,117 ???:<rustc_middle::ty::Ty as rustc_middle::ty::fold::TypeSuperFoldable>::super_fold_with::<rustc_middle::ty::subst::SubstFolder>
1,473,070 ???:<rustc_middle::ty::ParamEnvAnd<rustc_middle::traits::query::type_op::ProvePredicate> as rustc_trait_selection::traits::query::type_op::TypeOp>::fully_perform
-921,526 ???:<rustc_infer::infer::InferCtxt>::instantiate_nll_query_response_and_region_obligations::<()>
-649,700 ???:<&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_middle::ty::fold::TypeFoldable>::try_fold_with::<rustc_middle::ty::subst::SubstFolder>
462,905 ???:<alloc::rc::Rc<alloc::vec::Vec<rustc_ast::tokenstream::TokenTree>> as core::ops::drop::Drop>::drop
It seems there's more calls in type unification, but I don't see how this change would impact that. Let me know if you have any ideas. Until then, I'm marking this as triaged.
@rustbot label +perf-regression-triaged
This is definitely noise.
This PR is doing strictly less work than before, I was possibly expecting an improvement, but don't see how this could cause a slowdown.
I agree that this looks like noise. The code in and around fold_ty is hot and sensitive to minor changes in inlining. If you check "show non-relevant results" you see there are all sorts of ups and downs, and the net average is a 0.01% improvement. For keccak we see several 2%+ improvements, but for some reason the significance threshold for keccak has gotten really high and so these don't show up by default.
Oh interesting. That makes sense actually, keccak is pretty heavy on the MIR infrastructure so that's one of the ones I would've expected to disproportionately benefit from this
keccak has been kinda noisy recently, right ?
The significance thresholding didn't find this 5% regression relevant either.
