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

perf: linearity in HashMap.(insert|erase) #4372

Merged
merged 4 commits into from
Jun 7, 2024
Merged

Conversation

TwoFX
Copy link
Member

@TwoFX TwoFX commented Jun 6, 2024

The speedcenter doesn't seem to care much, but in my benchmark for replace-heavy workloads, the fixed code is about 40% faster.

@TwoFX
Copy link
Member Author

TwoFX commented Jun 6, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit f626d65.
There were no significant changes against commit 287d46e.

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc June 6, 2024 09:32 Inactive
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Jun 6, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Jun 6, 2024

Mathlib CI status (docs):

  • ❗ Batteries CI can not be attempted yet, as the nightly-testing-2024-06-06 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-mathlib, Batteries CI should run now. (2024-06-06 09:36:04)
  • ✅ Mathlib branch lean-pr-testing-4372 has successfully built against this PR. (2024-06-07 08:44:36) View Log
  • ✅ Mathlib branch lean-pr-testing-4372 has successfully built against this PR. (2024-06-07 11:28:29) View Log

@TwoFX
Copy link
Member Author

TwoFX commented Jun 6, 2024

!bench

@TwoFX TwoFX changed the title perf: linearity in HashMap.(insert|update) perf: linearity in HashMap.(insert|erase) Jun 6, 2024
@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 37e2bdb.
There were significant changes against commit 287d46e:

  Benchmark   Metric             Change
  ================================================
- stdlib      tactic execution     1.2% (1102.8 σ)

@TwoFX TwoFX marked this pull request as ready for review June 6, 2024 10:44
@leodemoura
Copy link
Member

@TwoFX Great catch. Have you considered using Array.modify?

@TwoFX
Copy link
Member Author

TwoFX commented Jun 7, 2024

@leodemoura I have, but I think it makes the code less clear in this case (because we already have bkt, so it's confusing to bind another name to it in the lambda passed to modify) and according to my benchmark there is no statistically significant difference in speed between the two variants. I'm happy to change it if you prefer the modify version (though note that this will make the diff larger because I would have to add a new definition HashMapBucket.modify).

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc June 7, 2024 07:38 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jun 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 7, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Jun 7, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc June 7, 2024 10:16 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jun 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 7, 2024
@leodemoura
Copy link
Member

@leodemoura I have, but I think it makes the code less clear in this case (because we already have bkt, so it's confusing to bind another name to it in the lambda passed to modify) and according to my benchmark there is no statistically significant difference in speed between the two variants. I'm happy to change it if you prefer the modify version (though note that this will make the diff larger because I would have to add a new definition HashMapBucket.modify).

Great. No need to change. Just wanted to know if you considered. I trust your judgment.

@leodemoura leodemoura added this pull request to the merge queue Jun 7, 2024
Merged via the queue into master with commit 2d05ff8 Jun 7, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants