Skip to content
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

[Merged by Bors] - perf(Topology.Algebra.Module): factor out smul #11331

Closed

Conversation

mattrobball
Copy link
Collaborator

We want to avoid making Lean unfold smul during unification. A separate instance does helps at the cost of some elaboration failures.


Open in Gitpod

@mattrobball
Copy link
Collaborator Author

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 3c14401.
There were significant changes against commit 9ddaa42:

  Benchmark                                             Metric         Change
  ===========================================================================
+ ~Mathlib.Analysis.Calculus.FDeriv.Mul                 instructions    -8.9%
+ ~Mathlib.Analysis.InnerProductSpace.Adjoint           instructions   -10.4%
+ ~Mathlib.Analysis.InnerProductSpace.Basic             instructions    -4.7%
+ ~Mathlib.Analysis.NormedSpace.OperatorNorm.Bilinear   instructions   -13.0%
+ ~Mathlib.Analysis.NormedSpace.Star.Multiplier         instructions    -5.9%
+ ~Mathlib.Geometry.Manifold.ContMDiff.NormedSpace      instructions   -12.6%

@mattrobball mattrobball added the awaiting-review The author would like community review of the PR label Mar 12, 2024
Comment on lines 341 to 342
@[simp, nolint simpNF] -- `simp` times out trying to find `Module ℂ (E →L[ℂ] ℂ)` but this fine
-- with all of `Mathlib` opened -- no idea why
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing a word?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Better english now hopefully

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Mar 24, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Mar 25, 2024
Copy link
Collaborator

@YaelDillies YaelDillies left a comment

Choose a reason for hiding this comment

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

That looks fine but what's the interaction with https://leanprover.zulipchat.com/#narrow/stream/144837-PR-reviews/topic/.2311521.20speeding.20up.20lots.20of.20instances ? Won't that discussion make this PR obsolete soon?

@jcommelin jcommelin added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Mar 26, 2024
@mattrobball
Copy link
Collaborator Author

fast_instsnces% doesn't chunk up it's input into new declarations and add them to the environment. It only looks for preexisting instances. So if you don't split off, SMul here there won't be any preexisting instances to find.

So it in fact works in tandem.

@mattrobball mattrobball added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Mar 26, 2024
@YaelDillies
Copy link
Collaborator

Okay, let's go

maintainer merge

Copy link

🚀 Pull request has been placed on the maintainer queue by YaelDillies.

Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

Thanks 🎉

bors merge

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Mar 26, 2024
mathlib-bors bot pushed a commit that referenced this pull request Mar 26, 2024
We want to avoid making Lean unfold `smul` during unification. A separate instance does helps at the cost of some elaboration failures.
@mathlib-bors
Copy link

mathlib-bors bot commented Mar 26, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title perf(Topology.Algebra.Module): factor out smul [Merged by Bors] - perf(Topology.Algebra.Module): factor out smul Mar 26, 2024
@mathlib-bors mathlib-bors bot closed this Mar 26, 2024
@mathlib-bors mathlib-bors bot deleted the mrb/factor_out_data_topology_algebra_module branch March 26, 2024 14:16
Louddy pushed a commit that referenced this pull request Apr 15, 2024
We want to avoid making Lean unfold `smul` during unification. A separate instance does helps at the cost of some elaboration failures.
atarnoam pushed a commit that referenced this pull request Apr 16, 2024
We want to avoid making Lean unfold `smul` during unification. A separate instance does helps at the cost of some elaboration failures.
uniwuni pushed a commit that referenced this pull request Apr 19, 2024
We want to avoid making Lean unfold `smul` during unification. A separate instance does helps at the cost of some elaboration failures.
callesonne pushed a commit that referenced this pull request Apr 22, 2024
We want to avoid making Lean unfold `smul` during unification. A separate instance does helps at the cost of some elaboration failures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer-merge ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants