Removing the def_id field from hot ParamEnv to make it smaller
#76244
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jackh726 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
vandenheuvel
force-pushed
the
remove__paramenv__def_id
branch
from
71cb92b
to
6f44404
Compare
compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
Outdated
Show resolved
Hide resolved
vandenheuvel
force-pushed
the
remove__paramenv__def_id
branch
from
6f44404
to
0d7ab10
Compare
| @@ -58,6 +58,7 @@ impl<'tcx> ExplicitPredicatesMap<'tcx> { | |||
| | ty::PredicateAtom::Subtype(..) | |||
| | ty::PredicateAtom::ConstEvaluatable(..) | |||
| | ty::PredicateAtom::ConstEquate(..) => (), | |||
| ty::PredicateAtom::TypeFromEnv(..) => unimplemented!(), | |||
There was a problem hiding this comment.
What would you think about changing this list (which is also a duplicate) to a _ pattern?
vandenheuvel
force-pushed
the
remove__paramenv__def_id
branch
from
0d7ab10
to
047f03f
Compare
vandenheuvel
force-pushed
the
remove__paramenv__def_id
branch
from
d88ca38
to
db6b482
Compare
|
Also, seems like one of the test outputs changed, so you'll have to |
compiler/rustc_ty/src/ty.rs
Outdated
| @@ -276,6 +281,113 @@ fn param_env(tcx: TyCtxt<'_>, def_id: DefId) -> ty::ParamEnv<'_> { | |||
| traits::normalize_param_env_or_error(tcx, def_id, unnormalized_env, cause) | |||
| } | |||
|
|
|||
| fn environment<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> &'tcx ty::List<Predicate<'tcx>> { | |||
There was a problem hiding this comment.
can we add a doc comment to this fn? it's pretty non-obvious to me what it does from its name. Actually, I suspect the name should just be improved to something like well_formed_types_in_env
There was a problem hiding this comment.
/// Elaborate the environment.
///
/// Collect a list of `Predicate`'s used for building the `ParamEnv`.
///
/// Used only in chalk mode.
@jackh726 anything to add?
There was a problem hiding this comment.
Perhaps mention that this adds TypeFromEnv (or what the eventual name ends up being) predicates. Specifically, because these types come from the environment, we assume they are well formed.
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 8f39a0415a6ca789a18c9d3bbbba43b5fda542bd with merge 5ef250dd2ad618ee339f165e9b711a1b4746887d... |
bors
added
the
S-waiting-on-bors
|
@bors r=nikomatsakis |
|
📌 Commit 7dad29d has been approved by |
|
☀️ Test successful - checks-actions, checks-azure |
|
It looks like this was a pretty major wall-time regression (up to 25%). There were some ~1% improvements too, but this regression seems to outweigh those. It looks like that was mostly in match checking, which is somewhat surprising -- there doesn't seem to be significant modifications to that code in this PR. I suspect we'll want to revert as this is a pretty big regression, but it would be useful to try to analyze where the instruction diff is coming from. |
|
@Mark-Simulacrum that's really strange, not what I would expect at all. Seems those 2-3% regressions in instruction count for unicode_normalization are pretty significant. I wonder if the additional |
|
Ah, I missed that there's two things being done here -- would've been good to split that into separate commits perhaps. @vandenheuvel do you think you could prepare a PR that reverts this one, and then just adds the PredicateAtom variant as @jackh726 suggests? |
|
@Mark-Simulacrum in hindsight, yeah, splitting into two makes sense. But given that we didn't expect a perf regression (since we were more focused on the removal of If @vandenheuvel can't, I can try to make a PR this weekend for perf. |
|
No worries! Yeah, we'll check in on this next week's perf triage (roughly Tuesday) but it's no big deal if it takes some time. |
|
Hi! This PR showed up in the weekly perf triage report. It resulted in a small improvement across the board in instruction counts. However, some benchmarks, notably Seems like this is already being looked into. Thanks all! |
Fixing the performance regression of rust-lang#76244 Issue rust-lang#74865 suggested that removing the `def_id` field from `ParamEnv` would improve performance. PR rust-lang#76244 implemented this change. Generally, [results](https://perf.rust-lang.org/compare.html?start=80fc9b0ecb29050d45b17c64af004200afd3cfc2&end=5ef250dd2ad618ee339f165e9b711a1b4746887d) were as expected: an instruction count decrease of about a percent. The instruction count for the unicode crates increased by about 3%, which `@nnethercote` speculated to be caused by a quirk of inlining or codegen. As the results were generally positive, and for chalk integration, this was also a step in the right direction, the PR was r+'d regardless. However, [wall-time performance results](https://perf.rust-lang.org/compare.html?start=a055c5a1bd95e029e9b31891db63b6dc8258b472&end=7402a394471a6738a40fea7d4f1891666e5a80c5&stat=task-clock) show a much larger performance degradation: 25%, as [mentioned](rust-lang#76244 (comment)) by `@Mark-Simulacrum.` This PR, for now, reverts rust-lang#76244 and attempts to find out, which change caused the regression.
|
@ecstatic-morse this issue is now picked up in #77058. |
…m-pat, r=Mark-Simulacrum Optimize `IntRange::from_pat`, then shrink `ParamEnv` Resolves rust-lang#77058. r? `@Mark-Simulacrum` cc `@vandenheuvel` Looking at the output of `perf report` for rust-lang#76244, the hot instructions seemed to be around the call to `pat_constructor` in `IntRange::from_pat`. I carried out an obvious optimization, but it actually made the instruction count higher (see rust-lang#77075). However, it seems to have mitigated whatever was causing the pipeline stalls, so when combined with rust-lang#76244, it's a net win. As you can see below, the regression in rust-lang#76244 seems to have originated from something measured by `stalled-cycles-backend`. I'll try to collect some finer-grained stats to see if I can isolate it. I wish I had a better idea of what was going on here. I'd like to prevent the regression from reappearing in the future due to small changes in unrelated code. <details> <summary>Current `master`:</summary> ``` Performance counter stats for 'cargo +baseline-stage1 check': 2,275.67 msec task-clock:u # 0.998 CPUs utilized 0 context-switches:u # 0.000 K/sec 0 cpu-migrations:u # 0.000 K/sec 49,826 page-faults:u # 0.022 M/sec 5,117,221,678 cycles:u # 2.249 GHz 299,655,943 stalled-cycles-frontend:u # 5.86% frontend cycles idle 2,284,213,395 stalled-cycles-backend:u # 44.64% backend cycles idle 8,051,871,959 instructions:u # 1.57 insn per cycle # 0.28 stalled cycles per insn 1,359,589,402 branches:u # 597.447 M/sec 7,359,347 branch-misses:u # 0.54% of all branches 2.281030026 seconds time elapsed 2.108197000 seconds user 0.164183000 seconds sys ``` </details> <details> <summary>Shrink `ParamEnv` without changing `IntRange::from_pat`:</summary> ``` Performance counter stats for 'cargo +perf-stage1 check': 2,751.79 msec task-clock:u # 0.996 CPUs utilized 0 context-switches:u # 0.000 K/sec 0 cpu-migrations:u # 0.000 K/sec 50,103 page-faults:u # 0.018 M/sec 6,260,590,019 cycles:u # 2.275 GHz 317,355,920 stalled-cycles-frontend:u # 5.07% frontend cycles idle 3,397,743,582 stalled-cycles-backend:u # 54.27% backend cycles idle 8,276,224,367 instructions:u # 1.32 insn per cycle # 0.41 stalled cycles per insn 1,370,453,386 branches:u # 498.023 M/sec 7,281,031 branch-misses:u # 0.53% of all branches 2.763265838 seconds time elapsed 2.544578000 seconds user 0.204548000 seconds sys ``` </details> <details> <summary>Shrink `ParamEnv` and change `IntRange::from_pat`: </summary> ``` Performance counter stats for 'cargo +perf-stage1 check': 2,295.57 msec task-clock:u # 0.996 CPUs utilized 0 context-switches:u # 0.000 K/sec 0 cpu-migrations:u # 0.000 K/sec 49,959 page-faults:u # 0.022 M/sec 5,151,407,066 cycles:u # 2.244 GHz 324,517,829 stalled-cycles-frontend:u # 6.30% frontend cycles idle 2,301,671,001 stalled-cycles-backend:u # 44.68% backend cycles idle 8,130,868,329 instructions:u # 1.58 insn per cycle # 0.28 stalled cycles per insn 1,356,618,512 branches:u # 590.972 M/sec 7,323,800 branch-misses:u # 0.54% of all branches 2.304509653 seconds time elapsed 2.128090000 seconds user 0.163909000 seconds sys ``` </details>
This PR addresses #74865.