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

WIP: tinker with Function.Surjective.module #6144

Closed
wants to merge 1 commit into from

Conversation

kbuzzard
Copy link
Member


Open in Gitpod

@eric-wieser
Copy link
Member

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 8b03d56.
There were significant changes against commit 03720bd:

  Benchmark                                     Metric         Change
  ===================================================================
- ~Mathlib.Algebra.Category.ModuleCat.Abelian   instructions     5.8%
- ~Mathlib.Algebra.Category.ModuleCat.Limits    instructions    15.7%

@kbuzzard kbuzzard added the WIP Work in progress label Jul 28, 2023
@kbuzzard
Copy link
Member Author

kbuzzard commented Jul 31, 2023

I wonder why this change has such a detrimental effect on those files? Note to self -- chase this up. Note to anyone else reading: I remove a "with" and then in contrast to the usual behaviour (either nothing, or a bunch of files go quicker), here a bunch of files go slower.

@kbuzzard
Copy link
Member Author

Mathlib.Algebra.Category.ModuleCat.Limits is 250 lines and compiles on my machine in 8 seconds, so a 15.7% increase is not the end of the world :-) Around half the time is spent on one declaration, directLimitsIsColimit. On master according to count_heartbeats this takes 140000-150000 heartbeats, and according to the profiler it takes about 5 seconds on my machine. Piping the trace output of the declaration to a file gives a 40952-line file.

With the change in this PR, the declaration now takes about 6 seconds, it goes up to 188426 heartbeats and the trace is now 49911 lines, an increase of 25%.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Aug 2, 2023
@kbuzzard
Copy link
Member Author

kbuzzard commented Aug 3, 2023

master now has the change that this PR makes because it was also (essentially) made in #6241 . Note that that PR was also benchmarked and similar slowdowns were observed in the same two files.

@kbuzzard kbuzzard closed this Aug 3, 2023
@kbuzzard kbuzzard deleted the kbuzzard-Function.Surjective.module branch February 24, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflict The PR has a merge conflict with master, and needs manual merging. WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants