Optimize try_evaluate_obligations

[nnethercote]

View all comments

This function is very sub-optimal, perf-wise: it takes self.obligations.pending (with mem::take) and iterates over the elements, checking each one. But most of the time no progress is made and all the obligations get pushed back onto self.obligations.pending. This drain + reconstruct approach is very expensive, mostly because the new pending vec is built by pushing one element at a time, which requires repeated reallocations. And this vec can have thousands of elements in it, in extreme cases.

Also, obligation and stalled_on get passed by value to evaluate_root_goal (obligation as goal), which then usually passes the values back in the GoalEvaluation which is immediately deconstructed. This is a lot of wasted value moves.

This commit optimizes things in two ways.

• It prioritizes the hot path. This involves checking in advance if there is an inspector (usually not) and adding goal_remains_stalled which takes stalled_on by reference. This hot path avoids all the value moves and GoalEvaluation construction/deconstruction and gets to the very common "nothing needed to be done" outcome as quickly as possible.

• It uses retain_mut to update self.obligations.pending. This requires some adjustments (e.g. handling recursion via the overflowed flag with some cleanup code after the retain_mut call, and cloning obligations in the error cases).

r? @lcnr
cc @jdonszelmann @WaffleLapkin

[rustbot]

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

[nnethercote]

On my Linux box this reduced the wall-time for a check full build of nacl-0.5.3 and ijson-0.1.6 by ~50%, and of nvml-wrapper-sys-0.9.1 by ~25%.

Local instruction count results for all the new-solver benchmarks (including those three, which I have added locally but aren't on CI):

[nnethercote]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 443ade9 (443ade9a155ac75fdadee81c20ae7a9b8576e119)
Base parent: c9ff496 (c9ff496891c278ad660bc0ab85c1f0b72059464a)

[rust-timer]

Finished benchmarking commit (443ade9): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-5.1%, -0.2%]	19
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.3%, secondary -0.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	4.4%	[4.4%, 4.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-1.2%, -0.4%]	16
Improvements ✅ (secondary)	-0.9%	[-2.6%, -0.4%]	26
All ❌✅ (primary)	-0.3%	[-1.2%, 4.4%]	17

Cycles

Results (primary -1.1%, secondary -2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.4%, 0.7%]	4
Regressions ❌ (secondary)	0.9%	[0.5%, 1.3%]	5
Improvements ✅ (primary)	-1.8%	[-7.0%, -0.5%]	10
Improvements ✅ (secondary)	-3.1%	[-11.1%, -0.4%]	19
All ❌✅ (primary)	-1.1%	[-7.0%, 0.7%]	14

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.838s -> 490.758s (0.19%)
Artifact size: 390.28 MiB -> 390.21 MiB (-0.02%)

[nnethercote]

The perf results on CI are a delight:

• icount reductions are a bit bigger than I saw locally across all new-solver benchmarks, e.g. -5.12% for wg-grammar on CI vs. -3.96% locally (for a check full build).
• cycles and wall-time both show a 10% reduction for a check full build of wg-grammar, and 1-6% across a majority of the other new-solver benchmarks.

[jdonszelmann]

r? me

[lcnr]

can throw that check out. There's no use in reinspecting a stalled goal as it hasn't changed since the last time :> inspectors only exist for external tools 😁

View changes since the review

[nnethercote]

Ok. I have added a second commit for this.

[nnethercote]

I have three follow-up changes that will improve these cases more.

[jdonszelmann]

note: the first thing evaluate_root_goal does is to check again goal_remains_stalled.

View changes since the review

[nnethercote]

Yes, there is some repeated work: rerunning_stalled_goal_may_make_progress can be called twice. It's idempotent and the wasted work doesn't matter. Here's some Cachegrind output:

          .                         // Common case: no inspector, still stalled; keep the obligation. This path is
          .                         // extremely hot in some cases; there can be thousands of pending obligations.
114,082,625 (0.8%)                  if !has_inspector
228,165,250 (1.6%)                      && let Some(stalled_on) = opt_stalled_on
          .                             && let Some(certainty) = delegate.goal_remains_stalled(stalled_on)
          .                             && matches!(certainty, Certainty::Maybe(_))
          .                         { 
          .                             return true;
          .                         }
          .
    622,248 (0.0%)                  let result = delegate.evaluate_root_goal(
          .                             obligation.as_goal(),
    155,562 (0.0%)                      obligation.cause.span,                                                                                   
          .                             opt_stalled_on.take(),                                                                                   
          .                         );      

The common case is more than 100x hotter than what follows. It would be possible to refactor evaluate_root_goal to avoid this wasted work, but evaluate_root_goal has four call sites and they would all need some changes and I don't think it's worthwhile.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[nnethercote]

I updated the code. I added the suggested comments. The other suggestions above were about inefficiencies on the cold path and I don't think anything needs changing there. Good to go?

[jdonszelmann]

I love it, lgtm!

@bors r+

[rust-bors]

📌 Commit 4b0dd6f has been approved by jdonszelmann

It is now in the queue for this repository.

[lcnr]

we should change stalled_on to contain a MaybeCause instead of a Certainty. It is always Certainty::Maybe

View changes since the review

[nnethercote]

As a follow-up?

[rust-bors]

💔 Test for 84aa78d failed: CI

[JonathanBrouwer]

hmmm
@bors retry