-
Notifications
You must be signed in to change notification settings - Fork 169
Add profiling and defeq guidance in style #764
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
Changes from 2 commits
34bb002
659e9c7
6384706
94a5675
5a523df
6e70dd0
e8ddedf
cd5738c
bf2831d
6b48953
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -520,6 +520,80 @@ There are two main reasons for this: | |
| 2. A squeezed `simp` call refers to many lemmas by name, meaning that it will break when one such | ||
| lemma gets renamed. Lemma renamings happen often enough for this to matter on a maintenance level. | ||
|
|
||
| ### Profiling for performance | ||
|
|
||
| When contributing to mathlib, authors should be aware of the performance impacts | ||
| of their contributions. The Lean FRO maintains benchmarking infrastructure which | ||
| can be accessed by commenting `!bench` on a PR. | ||
|
|
||
| Authors should assure that their contributions do not cause significant | ||
| performance regressions. In particular, if the PR touches significant components | ||
| of the language like adding instances, adding `simp` lemmas, changing imports, | ||
| or creating new definitions, then authors should benchmark their changes | ||
| proactively. | ||
|
|
||
| ### Transparency and API design | ||
|
|
||
| Central to Lean being a practially performant proof assistant is avoiding | ||
mattrobball marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| checking of definitional equality for very large terms. In the elaborator (the | ||
| component of the language that converts syntax to terms), the notion of | ||
| transparency is the main mechanism to avoid unfolding large definitions when | ||
| unnecessary. Excluding `opaque` definitions, there are three levels of | ||
| transparency: | ||
| - `reducible` definitions are always unfolded | ||
| - `semireducible`(default) definitions are usually not unfolding in main tactics like | ||
mattrobball marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| `rw` and `simp`, but can be unfolded with a little effort like explicitly | ||
| calling `rfl` or `erw`. Semireducible definitions are also not unfolded during the | ||
| computation of keys for storing instances in the instance cache or simp | ||
| lemmas in the simp cache. | ||
| - `irreducible` definitions are never unfolded unless the user explicitly | ||
| requests it. | ||
|
||
|
|
||
| `def` by default create `semireducible` definitions and `abbrev` creates | ||
mattrobball marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| `reducible` (and `inlined`) definitions. | ||
mattrobball marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| When designing definitions, an author should give thought to the transparency level of | ||
| definitions. Consider how exposing the underlying term of your definition will | ||
| affect instance search and simplification. The default for mathlib is that definitions | ||
| should be `semireducible` unless there is a good reason otherwise which should | ||
| be clearly articulated in the PR description. This imposes overhead on | ||
| contributors who will need to declare new instances of the form | ||
| ```lean4 | ||
| instance : Foo myDef := inferInstanceAs (Foo underlyingTermOfMyDef) | ||
| ``` | ||
| and recycle API lemmas, especially for `simp` use, like | ||
| ```lean4 | ||
| @[simp] lemma myDef_bar_eq_bizz (x : X) : myDef.bar = bizz := | ||
| underlyingTermOfMyDef_bar_eq_bizz | ||
| ``` | ||
|
|
||
| If the API boundary is meant to be completely sealed, using a type synonym of | ||
| the form | ||
| ```lean4 | ||
| structure myDef where | ||
| underlying : underlyingTerm | ||
| ``` | ||
| is the library convention in place of `irreducible` definitions. These are | ||
mattrobball marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
mattrobball marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| intended for types that are equivalent to an existing type but are clearly | ||
| mathematically semantically distinct, e.g. `Option` and `WithTop`. | ||
|
|
||
| The kernel does not have an analogous notion of transperency so its | ||
mattrobball marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| rules for unfolding are different. There are situations where an author wants | ||
| to block unfolding in the kernel as well. Mathlib provides a command | ||
| `irreducible_def` for this. This should be used only when there is a | ||
| documented necessity from profiling. | ||
|
|
||
| Use of `erw` or `rfl` after tactics like `simp` or `rw` that operate at | ||
| reducible transparency is an indication that there is missing API. | ||
| Consider adding the necessary lemmas to the API to avoid this. | ||
|
|
||
| The library has existing occurrences of definitional transparency abuse | ||
| like `erw` and extra `rfl`. PRs removing these are very welcome but their PR | ||
| description should clearly articulate how the removal is achieved addressing the | ||
| change in the underlying terms in particular and must benchmark their changes. | ||
| Please treat this an opportunity to improve the API design of the relevant | ||
| components. | ||
|
|
||
| ### Whitespace and delimiters | ||
|
|
||
| Lean is whitespace-sensitive, and in general we opt for a style which avoids | ||
|
|
||
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.
This is very broadly formulated. Do we really want to require a benchmark for every PR containing one of these? I think that in practice, most added instances or
simplemmas are for (very) specialized declarations with specific discrimination keys, so no performance regression would be expected.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.
Yes it is intentionally broad. The only real limit is benchmarking capacity. I don't see a way to draw it lower that it becomes effectively meaningless or convoluted. Suggestions welcome.
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.
How about:
"In particular, if the PR touches significant components of the library like adding generic instances, adding generic
simplemmas, changing imports or changing transparency levels of definitions, then authors should benchmark their changes proactively.""generic" is of course vague, and we could maybe say something more precise in terms of discrimination keys, but I am not sure this would help.
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.
A lot of things here would become simple with better automation. Checking for weak keys is one. I would keep the import switching because that is a completely inscrutable ball of yarn right now.
If this is meant for contributors that know little, then the default should be simple to understand and execute.