Skip to content

WIP: tinker with Function.Surjective.module#6144

Closed
kbuzzard wants to merge 1 commit intomasterfrom
kbuzzard-Function.Surjective.module
Closed

WIP: tinker with Function.Surjective.module#6144
kbuzzard wants to merge 1 commit intomasterfrom
kbuzzard-Function.Surjective.module

Conversation

@kbuzzard
Copy link
Copy Markdown
Member


Open in Gitpod

@eric-wieser
Copy link
Copy Markdown
Member

!bench

@leanprover-bot
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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%.

@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 2, 2023
@kbuzzard
Copy link
Copy Markdown
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. (this label is managed by a bot) WIP Work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants