Skip to content

perf: preprocess tc instances to avoid nested tc#3042

Closed
gebner wants to merge 2 commits intomasterfrom
tccommutes
Closed

perf: preprocess tc instances to avoid nested tc#3042
gebner wants to merge 2 commits intomasterfrom
tccommutes

Conversation

@gebner
Copy link
Copy Markdown
Member

@gebner gebner commented Mar 22, 2023


Open in Gitpod

@gebner
Copy link
Copy Markdown
Member Author

gebner commented Mar 22, 2023

!bench

Comment on lines +14 to +15
This file defines a `[@Commutes C inst₁ inst₂]` type class meaning that the
instances `inst₁ : C` and `inst₂ : C` are defeq. This allows us to write instances like:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Presumably this is enforced socially and not by lean itself; it looks to me like the typeclass only enforces that they're propositionally equal?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The only instance for Commutes witnesses definitional equality. It's not meant to be extended.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh wow that's a neat trick! I totally missed that Commutes.mk was an instance.

@gebner
Copy link
Copy Markdown
Member Author

gebner commented Mar 22, 2023

I view this PR as a slightly experimental hack, both to see how well the approach works, as well as something that we could maybe ship today (and most likely allow us to enable eta). The same issue arises with simp, and in aesop, and really anything that tries to apply lemmas in any form. It would be great if we could fix this directly in unification, but even if had a perfect solution today, it would take a couple of weeks to get it in.

open Lean Elab Parser Command in
/--
Similar to `instance`, but generalizes nonatomic instance terms in the
conclusion. Only use for `Prop`-valued classes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you enforce this with an error message?

@leanprover-bot
Copy link
Copy Markdown
Collaborator

Here are the benchmark results for commit 066538b.
The entire run failed.
Found no significant differences.

@gebner
Copy link
Copy Markdown
Member Author

gebner commented Mar 22, 2023

!bench

@leanprover-bot
Copy link
Copy Markdown
Collaborator

Here are the benchmark results for commit a7308f6.
There were significant changes against commit accbdce:

  Benchmark                                    Metric         Change
  ==================================================================
- ~Mathlib.Algebra.CovariantAndContravariant   instructions    33.8%
+ ~Mathlib.Algebra.GCDMonoid.Basic             instructions    -7.7%
+ ~Mathlib.Algebra.Module.Submodule.Lattice    instructions   -14.3%
+ ~Mathlib.Algebra.Order.AbsoluteValue         instructions    -6.0%
+ ~Mathlib.Algebra.Order.Group.Bounds          instructions   -11.3%
- ~Mathlib.Algebra.Order.Ring.WithTop          instructions     5.2%
- ~Mathlib.Algebra.Ring.OrderSynonym           instructions     7.2%
+ ~Mathlib.Algebra.Ring.Prod                   instructions    -7.2%
+ ~Mathlib.Data.Polynomial.Identities          instructions   -21.5%
+ ~Mathlib.Data.Real.ENatENNReal               instructions    -5.4%
+ ~Mathlib.GroupTheory.GroupAction.ConjAct     instructions   -10.6%
+ ~Mathlib.LinearAlgebra.Ray                   instructions   -11.6%
+ ~Mathlib.RingTheory.AlgebraTower             instructions   -21.4%
+ ~Mathlib.RingTheory.EuclideanDomain          instructions   -24.7%
+ ~Mathlib.RingTheory.Localization.Basic       instructions    -8.7%
+ ~Mathlib.RingTheory.Nilpotent                instructions    -9.9%
+ ~Mathlib.RingTheory.Polynomial.Chebyshev     instructions    -9.3%

@gebner
Copy link
Copy Markdown
Member Author

gebner commented Mar 23, 2023

That is a very specific improvement!

Unfortunately it is not enough to enable eta, as it doesn't solve Semiring { x // 0 ≤ x }, which still times out.

@kim-em kim-em added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Mar 23, 2023
@bors bors bot changed the base branch from master to ScottCarnahan/BinomialRing2 September 17, 2023 03:23
@kim-em kim-em changed the base branch from ScottCarnahan/BinomialRing2 to master September 17, 2023 12:18
@YaelDillies
Copy link
Copy Markdown
Contributor

I am closing this PR since it seems it has fulfilled its purpose. Feel free to reopen it.

@YaelDillies YaelDillies deleted the tccommutes branch November 28, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants