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: optimize Key.hash at DiscrTree for the .const constructor #6020

Closed
wants to merge 2 commits into from

Conversation

JovanGerb
Copy link
Contributor

This PR optimized Key.hash at for DiscrTree in the Key.const case. This is the most common case, and is computed regularly during type class search and simp.

@nomeata
Copy link
Collaborator

nomeata commented Nov 9, 2024

Do you have performance data that shows that this is relevant and worth the effort of a PR? (The reduced uniformity of the cases make me uneasy. Will this introduce new collisions with other constructors?)

@JovanGerb
Copy link
Contributor Author

Not yet, I'll bench it on mathlib when it builds

@JovanGerb
Copy link
Contributor Author

I don't really see how this could lead to more collisions. If you think it is better, I could move the mixHash 5237 $ over to the .proj constructor, since that one apparenly also doesn't have any explicit number in its hash function

@nomeata
Copy link
Collaborator

nomeata commented Nov 9, 2024

I'm not claiming to be an expert of this crucial code; if the performance gains are meausurable and significant then it shounds like it's worth for me to become one to be able to review it properly.

@nomeata
Copy link
Collaborator

nomeata commented Nov 9, 2024

(btw, if you want to run experiments before drawing attention from reviewers, then leaving the PR in draft mode signals that)

@JovanGerb JovanGerb marked this pull request as draft November 9, 2024 22:18
@JovanGerb
Copy link
Contributor Author

@nomeata, I guess we won't find out as there seems to be something weird going wrong with the build.

@JovanGerb JovanGerb closed this Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants