Skip to content

perf: try reverting #21330#25475

Closed
grunweg wants to merge 3 commits intomasterfrom
MR-aesopcat-rfl
Closed

perf: try reverting #21330#25475
grunweg wants to merge 3 commits intomasterfrom
MR-aesopcat-rfl

Conversation

@grunweg
Copy link
Collaborator

@grunweg grunweg commented Jun 5, 2025

By now, mathlib is on a Lean version including leanprover/lean4#7428 (for which #21330 was a workaround).
Let's measure the performance implications of reverting that change.


Open in Gitpod

@grunweg grunweg requested a review from JovanGerb June 5, 2025 10:29
@grunweg grunweg added awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. bench-after-CI Once the PR passes CI, comment `!bench` on the PR labels Jun 5, 2025
@github-actions github-actions bot added the t-category-theory Category theory label Jun 5, 2025
@github-actions
Copy link

github-actions bot commented Jun 5, 2025

PR summary 8277e451a7

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

No declarations were harmed in the making of this PR! 🐙

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.


No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@JovanGerb
Copy link
Collaborator

My suspicion is that the rfl_cat trick is still beneficial for performance, but that the impact will be less pronounced. Let's see!

@grunweg
Copy link
Collaborator Author

grunweg commented Jun 5, 2025

I'm not sure how to fix the last sorry (and cannot benchmark without that, it seems). Do you have an idea?

@JovanGerb
Copy link
Collaborator

The problem was that in kerAdjunction, the aesop tactic was for some reason using the variable U. This is really sneaky and evil, because as a result, the type of kerAdjunction has changed to include the argument U.

@grunweg
Copy link
Collaborator Author

grunweg commented Jun 5, 2025

The problem was that in kerAdjunction, the aesop tactic was for some reason using the variable U. This is really sneaky and evil, because as a result, the type of kerAdjunction has changed to include the argument U.

Oh wow. Thanks for catching and fixing this! (And I wonder if this issue also occurs in other places in mathlib... though I am not sure how to test for this.)

@ghost
Copy link

ghost commented Jun 5, 2025

!bench

@github-actions github-actions bot removed the awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. label Jun 5, 2025
@JovanGerb
Copy link
Collaborator

This can only happen in definitions, since theorems determine their type before elaborating the proof. And hopefully definitions get used, so that this kind of change gets caught immediately. But maybe with instances it can be less obvious when this problem arises.

@grunweg
Copy link
Collaborator Author

grunweg commented Jun 5, 2025

!bench

@grunweg grunweg added awaiting-bench This PR needs to be benchmarked before merging and removed bench-after-CI Once the PR passes CI, comment `!bench` on the PR labels Jun 5, 2025
@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 8277e45.
There were significant changes against commit c7f51d8:

  Benchmark                                                        Metric         Change
  ======================================================================================
- build                                                            aesop           20.3%
- ~Mathlib.Algebra.Category.Grp.LeftExactFunctor                   instructions    54.2%
- ~Mathlib.Algebra.Category.ModuleCat.Topology.Basic               instructions     7.9%
- ~Mathlib.AlgebraicGeometry.Morphisms.ClosedImmersion             instructions    45.6%
- ~Mathlib.AlgebraicTopology.SimplicialNerve                       instructions    42.1%
- ~Mathlib.AlgebraicTopology.SimplicialObject.Basic                instructions    11.2%
- ~Mathlib.CategoryTheory.Comma.StructuredArrow.Basic              instructions     7.2%
- ~Mathlib.CategoryTheory.Monoidal.Internal.Module                 instructions    27.7%
- ~Mathlib.CategoryTheory.Skeletal                                 instructions    77.0%
- ~Mathlib.Condensed.Discrete.LocallyConstant                      instructions    68.3%
- ~Mathlib.Order.Category.BoolAlg                                  instructions    48.7%
- ~Mathlib.Order.Category.FinBoolAlg                               instructions    60.1%
- ~Mathlib.Topology.Sheaves.SheafCondition.PairwiseIntersections   instructions    16.1%

grunweg added a commit that referenced this pull request Jun 5, 2025
Since mathlib includes leanprover/lean4#7448, update the comment accordingly.
#25475 shows that this fast-path is still beneficial: so leave it,
but update its comment accordingly.
@grunweg
Copy link
Collaborator Author

grunweg commented Jun 5, 2025

It seems this optimisation is still useful: thus, let's keep this (but only update the comment). #25482 does so.

@grunweg
Copy link
Collaborator Author

grunweg commented Jun 5, 2025

The performance changes does appear small than in #21330 which introduced it: but it's still significant enough, in my opinion.

@JovanGerb
Copy link
Collaborator

By the way, does the speedcenter website work for you? when I click the significant changes link, it doesn't manage to load the data.

@grunweg
Copy link
Collaborator Author

grunweg commented Jun 5, 2025

I get the same error.

@grunweg
Copy link
Collaborator Author

grunweg commented Jun 5, 2025

Now the comparison page loads fine: mathlib slows down by 600*10⁹ instructions (~0.44%) --- which is about a third of the original magnitude of #21330. In other words: it's great to confirm that this change is less required, but it's still worth keeping.

@grunweg grunweg closed this Jun 5, 2025
mathlib-bors bot pushed a commit that referenced this pull request Jun 15, 2025
Since mathlib includes leanprover/lean4#7448, update the comment accordingly. #25475 shows that this fast-path is still beneficial: so leave it, but update its comment accordingly.




Co-authored-by: grunweg <[email protected]>
@grunweg grunweg deleted the MR-aesopcat-rfl branch June 18, 2025 17:59
Rida-Hamadani pushed a commit to Rida-Hamadani/mathlib4 that referenced this pull request Jun 24, 2025
Since mathlib includes leanprover/lean4#7448, update the comment accordingly. leanprover-community#25475 shows that this fast-path is still beneficial: so leave it, but update its comment accordingly.




Co-authored-by: grunweg <[email protected]>
callesonne pushed a commit to callesonne/mathlib4 that referenced this pull request Jul 24, 2025
Since mathlib includes leanprover/lean4#7448, update the comment accordingly. leanprover-community#25475 shows that this fast-path is still beneficial: so leave it, but update its comment accordingly.




Co-authored-by: grunweg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-bench This PR needs to be benchmarked before merging t-category-theory Category theory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants