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: issue at binop% and binrel% elaborators #4085

Closed
wants to merge 1 commit into from

Conversation

leodemoura
Copy link
Member

This issue was affecting several Mathlib files.

@mattrobball Found where the [nonassignable]s were coming from. Thanks for helping to diagnose this performance issue.

@semorrison semorrison added the full-ci Run full CI suite label May 7, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc May 7, 2024 01:55 Inactive
@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 May 7, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

Mathlib CI status (docs):

  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 03040618b8f9b35b7b757858483e57340900cdc4 --onto b1bedbe0d2ba1dd3c654154897013e0399f5a53c. (2024-05-07 02:41:39)
  • 💥 Mathlib branch lean-pr-testing-4085 build failed against this PR. (2024-05-07 04:27:47) View Log
  • 💥 Mathlib branch lean-pr-testing-4085 build failed against this PR. (2024-05-07 05:12:55) View Log
  • 🟡 Mathlib branch lean-pr-testing-4085 build against this PR was cancelled. (2024-05-07 05:25:45) View Log
  • 🟡 Mathlib branch lean-pr-testing-4085 build this PR didn't complete normally. (2024-05-07 05:26:25) View Log
  • ✅ Mathlib branch lean-pr-testing-4085 has successfully built against this PR. (2024-05-07 06:15:00) View Log
  • ✅ Mathlib branch lean-pr-testing-4085 has successfully built against this PR. (2024-05-07 07:22:31) View Log

This issue was affecting several Mathlib files.
@mattrobball
Copy link
Contributor

At leanprover-community/mathlib4#12718, I benchmarked a working mathlib with a v4.8.0-rc1 with this commit cherry picked on top.

Overall there is a -400B in instructions driven mainly by one file which was -800B. The issue there was the new CoeT synthesis attempts in Lean.Elab.Term.Op.hasCoe.

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 7, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label May 7, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc May 7, 2024 04:30 Inactive
@semorrison
Copy link
Collaborator

semorrison commented May 7, 2024

The new slow-downs are pretty severe in Mathlib.AlgebraicGeometry.Restrict.

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 7, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added builds-mathlib CI has verified that Mathlib builds against this PR and removed breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan labels May 7, 2024
@semorrison
Copy link
Collaborator

semorrison commented May 7, 2024

I got different speed center results from leanprover-community/mathlib4#12719 than @mattrobball got from leanprover-community/mathlib4#12718; mine are worse, with an overall +500B instructions... The difference probably comes down to exactly where they converge with the master line in Mathlib; we're comparing against different base commits.

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc May 7, 2024 06:17 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 7, 2024
@mattrobball
Copy link
Contributor

It's universes triggering the expensive CoeT's.

mathlib-bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request May 7, 2024
Testing for leanprover/lean4#4085 exposed possible regressions typeclass synthesis which can be ameliorated using explicit universes. We do this and we add references to the issue #12737 tracking these changes.
@semorrison
Copy link
Collaborator

Closing in favour of #4092.

@semorrison semorrison closed this May 7, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 8, 2024
This issue was affecting several Mathlib files.

@mattrobball @semorrison This is a different solution for the issue. The
comment at `Extra.lean` describes the new solution and documents the new
issues found with the previous one.

closes #4085
github-actions bot pushed a commit that referenced this pull request May 8, 2024
This issue was affecting several Mathlib files.

@mattrobball @semorrison This is a different solution for the issue. The
comment at `Extra.lean` describes the new solution and documents the new
issues found with the previous one.

closes #4085

(cherry picked from commit 2a5ca00)
semorrison pushed a commit that referenced this pull request May 8, 2024
This issue was affecting several Mathlib files.

@mattrobball @semorrison This is a different solution for the issue. The
comment at `Extra.lean` describes the new solution and documents the new
issues found with the previous one.

closes #4085

(cherry picked from commit 2a5ca00)
apnelson1 pushed a commit to leanprover-community/mathlib4 that referenced this pull request May 12, 2024
Testing for leanprover/lean4#4085 exposed possible regressions typeclass synthesis which can be ameliorated using explicit universes. We do this and we add references to the issue #12737 tracking these changes.
callesonne pushed a commit to leanprover-community/mathlib4 that referenced this pull request May 16, 2024
Testing for leanprover/lean4#4085 exposed possible regressions typeclass synthesis which can be ameliorated using explicit universes. We do this and we add references to the issue #12737 tracking these changes.
semorrison pushed a commit that referenced this pull request May 21, 2024
This issue was affecting several Mathlib files.

@mattrobball @semorrison This is a different solution for the issue. The
comment at `Extra.lean` describes the new solution and documents the new
issues found with the previous one.

closes #4085

(cherry picked from commit 2a5ca00)
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 full-ci Run full CI suite toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants