Skip to content

[Merged by Bors] - perf: change to CommGroup instance on units#6398

Closed
kbuzzard wants to merge 3 commits intomasterfrom
kbuzzard-commgroup-units-speedup
Closed

[Merged by Bors] - perf: change to CommGroup instance on units#6398
kbuzzard wants to merge 3 commits intomasterfrom
kbuzzard-commgroup-units-speedup

Conversation

@kbuzzard
Copy link
Copy Markdown
Member

@kbuzzard kbuzzard commented Aug 6, 2023

Fixes a slowdown issue reported on Zulip. The first declaration in that example (the Basis example) takes 268578 to elaborate on master but only 11541 (an order of magnitude better) on this branch, and heartbeats no longer need to be bumped.


Open in Gitpod

@kbuzzard
Copy link
Copy Markdown
Member Author

kbuzzard commented Aug 6, 2023

!bench

@kbuzzard kbuzzard added the WIP Work in progress label Aug 6, 2023
@leanprover-bot
Copy link
Copy Markdown
Collaborator

Here are the benchmark results for commit e7b2be1.
There were significant changes against commit d3bfbe4:

  Benchmark                                        Metric           Change
  ========================================================================
- build                                            native linking     5.7%
+ ~Mathlib.CategoryTheory.Monad.Basic              instructions      -8.3%
+ ~Mathlib.NumberTheory.NumberField.Units          instructions     -19.5%
+ ~Mathlib.RingTheory.ClassGroup                   instructions      -5.9%
+ ~Mathlib.RingTheory.DedekindDomain.SelmerGroup   instructions     -42.0%

@kbuzzard kbuzzard added awaiting-review and removed WIP Work in progress labels Aug 6, 2023
@kbuzzard
Copy link
Copy Markdown
Member Author

kbuzzard commented Aug 6, 2023

(NB I think those heartbeats in the class group file could already be removed on master)

@Vierkantor
Copy link
Copy Markdown
Contributor

(NB I think those heartbeats in the class group file could already be removed on master)

My machine agrees, the number of used heartbeats is 56435 for ClassGroup.equiv and 51001 in ClassGroup.equiv_mk.

Copy link
Copy Markdown
Contributor

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Thanks!

bors r+

@ghost ghost added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Aug 7, 2023
bors bot pushed a commit that referenced this pull request Aug 7, 2023
Fixes a slowdown issue [reported on Zulip](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Quotient.20slowdowns/near/382006676). The first declaration in that example (the `Basis` example) takes 268578 to elaborate on master but only 11541 (an order of magnitude better) on this branch, and heartbeats no longer need to be bumped.
@bors
Copy link
Copy Markdown

bors bot commented Aug 7, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title perf: change to CommGroup instance on units [Merged by Bors] - perf: change to CommGroup instance on units Aug 7, 2023
@bors bors bot closed this Aug 7, 2023
@bors bors bot deleted the kbuzzard-commgroup-units-speedup branch August 7, 2023 15:21
kim-em pushed a commit that referenced this pull request Aug 14, 2023
Fixes a slowdown issue [reported on Zulip](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Quotient.20slowdowns/near/382006676). The first declaration in that example (the `Basis` example) takes 268578 to elaborate on master but only 11541 (an order of magnitude better) on this branch, and heartbeats no longer need to be bumped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has been sent to bors.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants