perf: assign synthesized instances directly when the goal type is ground#14330
Conversation
This PR makes `tryResolve` assign the goal metavariable directly after successfully unifying the goal type with the candidate instance type, instead of re-checking the types with `isDefEq`. The recheck is redundant for metavariable-free goals and can be expensive.
|
The experiment in #14318 was successful, but I need to clean up the PR and break it into smaller pieces. This is the first PR. |
|
!bench |
|
Benchmark results for 669b426 against e90c70f are in. There are significant results. @leodemoura
Large changes (1✅)
Medium changes (9✅)
and 1 hidden Small changes (757✅, 1🟥) Too many entries to display here. View the full report on radar instead. |
|
!bench mathlib |
|
Benchmark results for leanprover-community/mathlib4-nightly-testing@aa69f08 against leanprover-community/mathlib4-nightly-testing@55b60c6 are in. There are significant results. @leodemoura
Large changes (3✅)
and 1 hidden Medium changes (312✅) Too many entries to display here. View the full report on radar instead. Small changes (1129✅) Too many entries to display here. View the full report on radar instead. |
|
Reference manual CI status:
|
|
Mathlib CI status (docs):
|
Falsified by tests/elab/12172_regression.lean: MeasurableSpace-style classes are deliberately multi-instance, and eager synthesis commits goal metavariables that unification must determine later.
The previous commit's message is inaccurate: it describes an earlier experiment (synthesizing pending class metavariables at synthInstance entry) that was abandoned because it broke tests/elab/12172_regression.lean. What the commit actually implements is the final design: tryResolve assigns the synthesized instance directly (no recheck, no guard) and explicitly synthesizes the goal-type class metavariables that unification left undetermined (synthPendingClassMVars). This commit moves the accompanying repro from tests/leo/ (not run by ctest) into tests/elab/ and updates its docstring to describe the current code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
!bench |
|
Benchmark results for 0eb00fd against e90c70f are in. There are significant results. @leodemoura
No significant changes detected. |
|
!bench mathlib |
|
Benchmark results for leanprover-community/mathlib4-nightly-testing@aa69f08 against leanprover-community/mathlib4-nightly-testing@55b60c6 are in. (These commits have already been benchmarked in a previous command.) There are significant results. @leodemoura
Large changes (3✅)
and 1 hidden Medium changes (312✅) Too many entries to display here. View the full report on radar instead. Small changes (1129✅) Too many entries to display here. View the full report on radar instead. |
This PR restores the `isDefEq mvar instVal` recheck at the end of `tryResolve` for goals whose type bodies contain metavariables, keeping direct assignment only for metavariable-free goals, and documents why the recheck is load-bearing. The recheck's unification side effects synthesize pending class metavariables in the goal (e.g. `IsPredArchimedean ι ?pre ?pd`, created when elaborating class projections) via `isDefEqArgs`'s `trySynthPending` pass. The synthesized set depends on how the two compared types are spelled, so it cannot be recomputed after a direct assignment: synthesizing all pending class metavariables breaks stage2 (a higher-order `[Nonempty ε]` metavariable in `Init/Internal/Order/Basic.lean` must be left to unification), and synthesizing none breaks Mathlib (`Mathlib/Order/SuccPred/LinearLocallyFinite.lean`). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
!bench |
|
Benchmark results for 7d378d7 against e90c70f are in. There are significant results. @leodemoura
Large changes (1✅)
Medium changes (11✅)
and 1 hidden Small changes (755✅) Too many entries to display here. View the full report on radar instead. |
|
!bench mathlib |
|
Benchmark results for leanprover-community/mathlib4-nightly-testing@541fcad against leanprover-community/mathlib4-nightly-testing@55b60c6 are in. There are significant results. @leodemoura
Large changes (3✅)
and 1 hidden Medium changes (312✅) Too many entries to display here. View the full report on radar instead. Small changes (1132✅) Too many entries to display here. View the full report on radar instead. |
…und (#14330) This PR makes `tryResolve` assign the goal metavariable directly after successfully unifying the goal type with the candidate instance type, instead of re-checking the types with `isDefEq`. The recheck is redundant for metavariable-free goals and can be expensive. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This PR makes
tryResolveassign the goal metavariable directly after successfully unifying the goal type with the candidate instance type, instead of re-checking the types withisDefEq. The recheck is redundant for metavariable-free goals and can be expensive.