-
Notifications
You must be signed in to change notification settings - Fork 368
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(Tactic/FieldSimp): exclude simp lemmas that don't apply for fields. #21326
base: master
Are you sure you want to change the base?
Conversation
!bench |
PR summary 84caac5752Import changes for modified filesNo significant changes to the import graph Import changes for all files
Declarations diff
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
|
Here are the benchmark results for commit 84caac5. Benchmark Metric Change
======================================================================================
+ ~Mathlib.AlgebraicGeometry.EllipticCurve.Affine instructions -8.7%
+ ~Mathlib.AlgebraicGeometry.EllipticCurve.Jacobian instructions -4.9%
+ ~Mathlib.AlgebraicGeometry.EllipticCurve.Projective instructions -4.0%
+ ~Mathlib.Analysis.Convex.Slope instructions -19.3%
+ ~Mathlib.Analysis.Convex.Visible instructions -18.5%
+ ~Mathlib.Analysis.SpecialFunctions.Gamma.Deligne instructions -29.6%
+ ~Mathlib.Analysis.SpecialFunctions.Trigonometric.EulerSineProd instructions -31.1% |
40 files, Instructions -2.0⬝10⁹
16 files, Instructions -3.0⬝10⁹
7 files, Instructions -4.0⬝10⁹
9 files, Instructions -5.0⬝10⁹
5 files, Instructions -6.0⬝10⁹
4 files, Instructions -7.0⬝10⁹
3 files, Instructions -8.0⬝10⁹
2 files, Instructions -14.0⬝10⁹
2 files, Instructions -16.0⬝10⁹
|
These are impressive gains, thanks a lot! I can take a more detailed look at the list of lemmas later; in the meantime, I was wondering about the followoing:
I had some PRs un-expanding field_simp calls; I'll test on top of this PR. |
For the list of lemmas, I particularly picked all lemmas involving Yes, it would be more robust against regression if |
This PR excludes simp lemmas like
div_self'
from being used in thefield_simp
tactic. These lemmas have discrimination tree keys that commonly find a match when runningfield_simp
, and if they do match then unification needs to fail, because the field is in fact not aGroup
/CommGroup
/LeftCancelMonoid
/RightCancelMonoid
.