-
Notifications
You must be signed in to change notification settings - Fork 421
perf: make the support of onFinset opaque #24944
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
base: master
Are you sure you want to change the base?
Conversation
PR summary 076d55acf7Import 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
|
!bench |
1 similar comment
!bench |
Here are the benchmark results for commit 076d55a. Benchmark Metric Change
=========================================================================
+ ~Mathlib.Algebra.Category.ModuleCat.Presheaf.Free instructions -75.1%
+ ~Mathlib.Algebra.SkewMonoidAlgebra.Basic instructions -38.8% |
5 files, Instructions -2.0⬝10⁹
2 files, Instructions -3.0⬝10⁹
2 files, Instructions -4.0⬝10⁹
|
@@ -215,36 +215,40 @@ section OnFinset | |||
|
|||
variable [Zero M] | |||
|
|||
private irreducible_def onFinset_support (s : Finset α) (f : α → M) : Finset α := |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the kernel problematic here or just the elaborator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. It would indeed be a good test to make this just @[irreducible]
and see if the performance is any different. Do you mind trying that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can but I don't think I will have time until early next week.
It's very handy to have defeq of the function within a Finsupp, but we almost never care about the defeq of the support, especially since it almost always involves
Classical.decidable
which doesn't reduce.This mplements my idea from #mathlib4 > simp timeout at `whnf` @ 💬, aimed at making
(mapRange _ _ _).support
irreducible.This breaks the defeq of
0 = 0 + 0
on finsupp, but I think that's ok.