Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: avoid negative environment lookup #5429

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

Kha
Copy link
Member

@Kha Kha commented Sep 23, 2024

Avoids some Environment.find? lookup misses that become especially expensive on the async branch

@Kha
Copy link
Member Author

Kha commented Sep 23, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit f4edcbd.
The entire run failed.
Found no significant differences.

@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Sep 23, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Sep 23, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 23, 2024
@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Sep 23, 2024
@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

@Kha
Copy link
Member Author

Kha commented Sep 23, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 7347fd5.
There were significant changes against commit e551a36:

  Benchmark          Metric       Change
  ================================================
+ lake build clean   task-clock    -2.7% (-30.3 σ)

@@ -75,7 +75,7 @@ def isAuxDef (constName : Name) : MetaM Bool := do
-- ===========================

private def getFirstCtor (d : Name) : MetaM (Option Name) := do
let some (ConstantInfo.inductInfo { ctors := ctor::_, ..}) ← getUnfoldableConstNoEx? d |
let some (ConstantInfo.inductInfo { ctors := ctor::_, ..}) := (← getEnv).find? d |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change, and we should merge it as a 'fix' in a separate PR. Using getUnfoldedConstNoEx? here looks very strange, so I decided to trace its origin. It originates from Lean 3, where a function for retrieving a constant without throwing an exception was used. Since then, the code has been modified and refactored a few times, but this line was never addressed and has become increasingly odd.

@@ -39,7 +39,10 @@ def getUnfoldableConst? (constName : Name) : MetaM (Option ConstantInfo) := do
match (← getEnv).find? constName with
| some (info@(.thmInfo _)) => getTheoremInfo info
| some (info@(.defnInfo _)) => if (← canUnfold info) then return info else return none
| some info => return some info
| some (info@(.recInfo _)) => return some info
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this function is strange. It is currently called getUnfoldableConst? but some of the cases do not even have something to unfold (e.g., recInfo).
I found two use cases for this function:

  • isDeltaCandidate?: This one immediately checks whether the info has a value or not. We can optimize this for this case and have clean code.
  • matchConstAux: which is used to handle orthogonal cases using the same API. We should cleanup this one. This function looks odd to me.

I prefer to cleanup and ensure the function names make sense. If we decide to not cleanup, we should add comments explaining why the function implementation is counterintuitive.

@Kha Kha force-pushed the push-krpzlqwzkrqu branch 2 times, most recently from b396bdf to 7fa7c46 Compare November 7, 2024 14:25
@Kha Kha changed the title perf: optimize unfolding constant lookup perf: avoid negative environment lookup Nov 7, 2024
@Kha
Copy link
Member Author

Kha commented Nov 7, 2024

@leodemoura Thanks for the additional context, I reduced this PR to the reordering of operations and will rework getUnfoldableConst? in a separate PR

@Kha
Copy link
Member Author

Kha commented Nov 7, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 7fa7c46.
There were significant changes against commit e551a36:

  Benchmark                 Metric               Change
  ===============================================================
- bv_decide_mul             branch-misses          8.1% (662.4 σ)
- bv_decide_realworld       branch-misses          7.6%  (75.8 σ)
- ilean roundtrip           branch-misses        212.9% (122.8 σ)
- import Lean               branch-misses        349.9% (757.0 σ)
- import Lean               branches               3.2%  (67.4 σ)
- import Lean               instructions           1.6%  (28.6 σ)
- lake build clean          instructions           1.7%  (55.3 σ)
- lake config import        instructions           1.4%  (21.8 σ)
- lake config tree          instructions           1.3%  (32.1 σ)
- lake env                  instructions           1.4%  (80.6 σ)
- language server startup   branch-misses        157.8% (280.5 σ)
- language server startup   task-clock            12.2%  (29.6 σ)
- language server startup   wall-clock            10.7% (767.2 σ)
- nat_repr                  branch-misses        264.2% (140.2 σ)
- rbmap_fbip                task-clock             5.6%  (10.5 σ)
- rbmap_fbip                wall-clock             5.7%  (10.2 σ)
+ reduceMatch               task-clock            -2.6% (-44.6 σ)
+ reduceMatch               wall-clock            -2.7% (-35.5 σ)
+ stdlib                    dsimp                 -2.0% (-60.6 σ)
- stdlib                    share common exprs     1.6%  (13.7 σ)
+ stdlib                    tactic execution      -1.2% (-11.8 σ)
- stdlib                    task-clock             3.1%  (50.9 σ)
+ workspaceSymbols          task-clock            -5.1% (-36.7 σ)
+ workspaceSymbols          wall-clock            -5.1% (-36.2 σ)

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 7, 2024
@Kha Kha force-pushed the push-krpzlqwzkrqu branch from 7fa7c46 to 489c87a Compare November 7, 2024 14:47
@Kha
Copy link
Member Author

Kha commented Nov 7, 2024

!bench

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 7, 2024
@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 489c87a.
There were significant changes against commit c779f3a:

  Benchmark             Metric       Change
  ===================================================
+ bv_decide_realworld   task-clock    -2.6% (-18.8 σ)
- rbmap_10              task-clock     2.8%  (10.8 σ)
- rbmap_10              wall-clock     2.8%  (10.5 σ)

@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

@Kha Kha added the will-merge-soon …unless someone speaks up label Nov 7, 2024
@Kha
Copy link
Member Author

Kha commented Nov 7, 2024

Apparently this change is neutral until parallelism comes in, but I believe it is harmless enough now to go ahead with merging it

@Kha Kha marked this pull request as ready for review November 8, 2024 15:37
@Kha Kha added this pull request to the merge queue Nov 8, 2024
Merged via the queue into leanprover:master with commit dac73c1 Nov 8, 2024
16 checks passed
@Kha Kha deleted the push-krpzlqwzkrqu branch November 12, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN will-merge-soon …unless someone speaks up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants