[Merged by Bors] - refactor: mixin class for norm_smul with strict equality#24003
Closed
eric-wieser wants to merge 27 commits intomasterfrom
Closed
[Merged by Bors] - refactor: mixin class for norm_smul with strict equality#24003eric-wieser wants to merge 27 commits intomasterfrom
norm_smul with strict equality#24003eric-wieser wants to merge 27 commits intomasterfrom
Conversation
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Let's see whether this alone has an affect on the benchmark
PR summary 9c1b3099b5Import changes for modified filesNo significant changes to the import graph Import changes for all files
Declarations diffNo declarations were harmed in the making of this PR! 🐙 You can run this locally as follows## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>
## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>The doc-module for No changes to technical debt.You can run this locally as
|
Member
Author
|
!bench |
Collaborator
|
Here are the benchmark results for commit 0ac28ed. Benchmark Metric Change
===========================================================================
- ~Mathlib.Analysis.Analytic.Basic instructions 17.1%
- ~Mathlib.Analysis.Analytic.CPolynomialDef instructions 18.8%
- ~Mathlib.Analysis.Analytic.ChangeOrigin instructions 10.2%
- ~Mathlib.Analysis.Analytic.Constructions instructions 13.0%
- ~Mathlib.Analysis.Analytic.Inverse instructions 8.8%
- ~Mathlib.Analysis.Calculus.ContDiff.Basic instructions 11.4%
- ~Mathlib.Analysis.Calculus.ContDiff.Defs instructions 14.9%
- ~Mathlib.Analysis.Calculus.ContDiff.FTaylorSeries instructions 10.6%
- ~Mathlib.Analysis.Calculus.ContDiff.FaaDiBruno instructions 9.0%
- ~Mathlib.Analysis.Calculus.ContDiff.Operations instructions 15.3%
- ~Mathlib.Analysis.Calculus.Deriv.Basic instructions 12.7%
- ~Mathlib.Analysis.Calculus.Deriv.Mul instructions 9.8%
- ~Mathlib.Analysis.Calculus.FDeriv.Analytic instructions 7.0%
- ~Mathlib.Analysis.Calculus.FDeriv.Mul instructions 3.0%
- ~Mathlib.Analysis.Convolution instructions 5.8%
- ~Mathlib.Analysis.NormedSpace.Multilinear.Basic instructions 3.9%
- ~Mathlib.Analysis.NormedSpace.Multilinear.Curry instructions 5.2%
- ~Mathlib.Analysis.NormedSpace.OperatorNorm.Bilinear instructions 6.7% |
2 files, Instructions +30.0⬝10⁹
2 files, Instructions +21.0⬝10⁹
2 files, Instructions +19.0⬝10⁹
3 files, Instructions +11.0⬝10⁹
2 files, Instructions +10.0⬝10⁹
2 files, Instructions +8.0⬝10⁹
3 files, Instructions +5.0⬝10⁹
4 files, Instructions +4.0⬝10⁹
8 files, Instructions +3.0⬝10⁹
7 files, Instructions +2.0⬝10⁹
22 files, Instructions +1.0⬝10⁹
7 files, Instructions -2.0⬝10⁹
|
Member
|
I can see that certain things are slower in traces on this branch. For example takes 1479817 mHeartbeats on master and 1866830 mHeartbeats on this branch (a 25% slowdown). Matt has suggested on the Zulip thread #mathlib4 > Normed modules @ 💬 that there are now two paths to on this branch vs on master. Digging further into the slow trace the only times things don't look syntactically identical is right at the end: where Lean has made two ring structures on |
Contributor
|
Thanks Eric for making this PR! I am going to set #23787 to draft, and this PR to non-draft, since it contains the "first stage" of what I planned for that PR. Thanks to everyone who has contributed so far to trying to diagnose the slow-downs! |
loefflerd
changed the title
norm_smul with strict equality (alternative)norm_smul with strict equality
Member
Author
|
!bench |
Collaborator
|
Here are the benchmark results for commit 1007e3a. Benchmark Metric Change
===========================================================================
- ~Mathlib.Analysis.Analytic.Basic instructions 16.9%
- ~Mathlib.Analysis.Analytic.CPolynomialDef instructions 19.0%
- ~Mathlib.Analysis.Analytic.ChangeOrigin instructions 9.5%
- ~Mathlib.Analysis.Analytic.Constructions instructions 12.9%
- ~Mathlib.Analysis.Analytic.Inverse instructions 8.8%
- ~Mathlib.Analysis.Calculus.ContDiff.Basic instructions 11.3%
- ~Mathlib.Analysis.Calculus.ContDiff.Defs instructions 14.8%
- ~Mathlib.Analysis.Calculus.ContDiff.FTaylorSeries instructions 10.6%
- ~Mathlib.Analysis.Calculus.ContDiff.FaaDiBruno instructions 9.3%
- ~Mathlib.Analysis.Calculus.ContDiff.Operations instructions 15.3%
- ~Mathlib.Analysis.Calculus.Deriv.Basic instructions 12.6%
- ~Mathlib.Analysis.Calculus.Deriv.Mul instructions 9.8%
- ~Mathlib.Analysis.Calculus.FDeriv.Analytic instructions 7.0%
- ~Mathlib.Analysis.Calculus.FDeriv.Mul instructions 3.0%
- ~Mathlib.Analysis.Convolution instructions 5.7%
- ~Mathlib.Analysis.NormedSpace.Multilinear.Basic instructions 4.0%
- ~Mathlib.Analysis.NormedSpace.Multilinear.Curry instructions 5.2%
- ~Mathlib.Analysis.NormedSpace.OperatorNorm.Bilinear instructions 6.7% |
2 files, Instructions +21.0⬝10⁹
2 files, Instructions +19.0⬝10⁹
3 files, Instructions +11.0⬝10⁹
2 files, Instructions +10.0⬝10⁹
2 files, Instructions +8.0⬝10⁹
3 files, Instructions +5.0⬝10⁹
4 files, Instructions +4.0⬝10⁹
9 files, Instructions +3.0⬝10⁹
5 files, Instructions +2.0⬝10⁹
23 files, Instructions +1.0⬝10⁹
7 files, Instructions -2.0⬝10⁹
|
Member
Author
|
!bench |
Collaborator
|
Here are the benchmark results for commit 559a5ff. Benchmark Metric Change
===========================================================================
- ~Mathlib.Analysis.Analytic.Basic instructions 16.8%
- ~Mathlib.Analysis.Analytic.CPolynomialDef instructions 18.6%
- ~Mathlib.Analysis.Analytic.ChangeOrigin instructions 9.6%
- ~Mathlib.Analysis.Analytic.Constructions instructions 12.8%
- ~Mathlib.Analysis.Analytic.Inverse instructions 8.7%
- ~Mathlib.Analysis.Calculus.ContDiff.Basic instructions 11.2%
- ~Mathlib.Analysis.Calculus.ContDiff.Defs instructions 14.7%
- ~Mathlib.Analysis.Calculus.ContDiff.FTaylorSeries instructions 10.4%
- ~Mathlib.Analysis.Calculus.ContDiff.FaaDiBruno instructions 9.2%
- ~Mathlib.Analysis.Calculus.ContDiff.Operations instructions 15.1%
- ~Mathlib.Analysis.Calculus.Deriv.Basic instructions 12.9%
- ~Mathlib.Analysis.Calculus.Deriv.Mul instructions 9.7%
- ~Mathlib.Analysis.Calculus.FDeriv.Analytic instructions 7.0%
- ~Mathlib.Analysis.Calculus.FDeriv.Mul instructions 3.0%
- ~Mathlib.Analysis.Convolution instructions 5.6%
- ~Mathlib.Analysis.NormedSpace.Multilinear.Basic instructions 3.9%
- ~Mathlib.Analysis.NormedSpace.Multilinear.Curry instructions 5.3%
- ~Mathlib.Analysis.NormedSpace.OperatorNorm.Bilinear instructions 6.6% |
2 files, Instructions +30.0⬝10⁹
2 files, Instructions +21.0⬝10⁹
2 files, Instructions +19.0⬝10⁹
2 files, Instructions +12.0⬝10⁹
3 files, Instructions +11.0⬝10⁹
2 files, Instructions +10.0⬝10⁹
2 files, Instructions +8.0⬝10⁹
4 files, Instructions +5.0⬝10⁹
3 files, Instructions +4.0⬝10⁹
6 files, Instructions +3.0⬝10⁹
8 files, Instructions +2.0⬝10⁹
22 files, Instructions +1.0⬝10⁹
7 files, Instructions -2.0⬝10⁹
2 files, Instructions -3.0⬝10⁹
|
Member
Author
Member
|
For me, yes |
Contributor
|
This looks ready to me (I am also happy with the commit message, fwiw, although my opinion was not asked!). Shall we merge? |
Member
Author
|
@kbuzzard plans to look at the performance one last time in the next few days, but after that, hopefully yes! |
Member
|
OK my final round of thoughts on this PR are #mathlib4 > Normed modules @ 💬 and I feel like we're going to converge shortly. Basically I propose what I think is a speedup involving a typeclass shortcut and I'd be interested to hear anyone's opinion as to whether it's acceptable or whether it's a hack. |
Member
Author
|
!bench |
Collaborator
|
Here are the benchmark results for commit 71b5341. |
Member
Author
|
!bench |
Collaborator
|
Here are the benchmark results for commit 9c1b309. |
2 files, Instructions +2.0⬝10⁹
15 files, Instructions -2.0⬝10⁹
2 files, Instructions -3.0⬝10⁹
|
Member
|
Thanks everyone for your patience! bors merge |
ghost
added
the
kbuzzard
removed
delegated
Member
|
Hmm, I just realised that the description of the PR is now out of date (it describes a problem which no longer exists...). I thought that perhaps moving quickly was the best approach so I cut and pasted the comments "below the line". Sorry for the confusion. |
Member
Author
|
bors merge |
mathlib-bors bot
pushed a commit
that referenced
this pull request
May 8, 2025
Define a typeclass for normed modules where the norm is exactly multiplicative (not just sub-multiplicative like `IsBoundedSMul`, or just over fields like `NormedSpace`). This is a subset of #23787, leaving out the changes to `NormedSpace`. Co-authored-by: Eric Wieser <efw@google.com> Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Contributor
|
Pull request successfully merged into master. Build succeeded: |
mathlib-bors
bot
changed the title
norm_smul with strict equalitynorm_smul with strict equality
tannerduve
pushed a commit
that referenced
this pull request
May 13, 2025
Define a typeclass for normed modules where the norm is exactly multiplicative (not just sub-multiplicative like `IsBoundedSMul`, or just over fields like `NormedSpace`). This is a subset of #23787, leaving out the changes to `NormedSpace`. Co-authored-by: Eric Wieser <efw@google.com> Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
bwehlin
pushed a commit
to bwehlin/mathlib4
that referenced
this pull request
May 31, 2025
…r-community#24003) Define a typeclass for normed modules where the norm is exactly multiplicative (not just sub-multiplicative like `IsBoundedSMul`, or just over fields like `NormedSpace`). This is a subset of leanprover-community#23787, leaving out the changes to `NormedSpace`. Co-authored-by: Eric Wieser <efw@google.com> Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Define a typeclass for normed modules where the norm is exactly multiplicative (not just sub-multiplicative like
IsBoundedSMul, or just over fields likeNormedSpace).This is a subset of #23787, leaving out the changes to
NormedSpace.The comments below used to be in the PR description, but future changes mean that this is no longer true; the performance drop was fixed.
This results in a reasonably significant performance drop (~0.5T instructions or 0.3%), but no obvious tweaking of priorities seems to help much. Some further discussion took place in #mathlib4 > Normed modules @ 💬, though for now there are no actionable suggestions without further experimentation.
A somewhat realistic benchmark can be constructed as
where the number of heartbeats changes from
1477626to1858556(+25%) due to this PR.