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: fix implementation of move constructors and move assignment ope… #4700

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

legrosbuffle
Copy link
Contributor

@legrosbuffle legrosbuffle commented Jul 9, 2024

…rators

Right now those constructors result in a copy instead of the desired move. We've measured that expr copying and assignment by itself uses around 10% of total runtime on our workloads.

See #4698 for details.

…rators

Right now those constructors result in a copy instead of the desired
move. We've measured that expr copying and assignment by itself uses
around 10% of total runtime on our workloads.

See leanprover#4698 for details.
@legrosbuffle legrosbuffle requested a review from leodemoura as a code owner July 9, 2024 12:46
@legrosbuffle legrosbuffle marked this pull request as draft July 9, 2024 12:55
@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 Jul 9, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Jul 9, 2024

Mathlib CI status (docs):

  • ❗ Batteries CI can not be attempted yet, as the nightly-testing-2024-07-09 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-mathlib, Batteries CI should run now. (2024-07-09 13:02:53)
  • 💥 Mathlib branch lean-pr-testing-4700 build failed against this PR. (2024-07-10 01:14:57) View Log
  • ✅ Mathlib branch lean-pr-testing-4700 has successfully built against this PR. (2024-07-10 16:12:49) View Log

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit c3c46b2.
There were significant changes against commit 582d6e7:

  Benchmark     Metric             Change
  ==================================================
+ reduceMatch   instructions        -1.4% (-352.1 σ)
+ stdlib        tactic execution    -1.4% (-342.6 σ)

@kim-em kim-em added the release-ci Enable all CI checks for a PR, like is done for releases label Jul 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jul 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jul 10, 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 Jul 10, 2024
@kim-em
Copy link
Collaborator

kim-em commented Jul 10, 2024

Mathlib benchmark is hopefully running now at leanprover-community/mathlib4#14599.

@kim-em
Copy link
Collaborator

kim-em commented Jul 10, 2024

Mathlib benchmark is hopefully running now at leanprover-community/mathlib4#14599.

No significant changes in Mathlib build time. (This is, of course, not intended to be a representative benchmark!)

@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 Jul 10, 2024
@Kha
Copy link
Member

Kha commented Jul 14, 2024

@legrosbuffle I assume the time savings happen in kernel type checking in your case? While lean4 and mathlib4 are not bottlenecked by the kernel at the moment, we are still interested in optimizing it. Do you think you could extract a representative stand-alone test case for the bottleneck and speedup?

@legrosbuffle
Copy link
Contributor Author

@legrosbuffle I assume the time savings happen in kernel type checking in your case?

Yes, the majority of the cost if from src/kernel/replace_fn.cpp.

While lean4 and mathlib4 are not bottlenecked by the kernel at the moment, we are still interested in optimizing it. Do you think you could extract a representative stand-alone test case for the bottleneck and speedup?

It's not obvious that this is easy to reduce, but I'll see what I can do.

@leanprover-bot leanprover-bot added the P-high We will work on this issue label Aug 2, 2024
@leodemoura leodemoura marked this pull request as ready for review August 2, 2024 17:45
@leodemoura leodemoura added this pull request to the merge queue Aug 2, 2024
Merged via the queue into leanprover:master with commit 2c00271 Aug 2, 2024
32 checks passed
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 P-high We will work on this issue release-ci Enable all CI checks for a PR, like is done for releases 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.

6 participants