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

Speed up equality; remove expensive invariant #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

treeowl
Copy link
Contributor

@treeowl treeowl commented May 16, 2023

We used to define == to test equality in priority order. This was expensive in and of itself, since it takes logarithmic time to produce each entry. But the situation was worse than that: since priorities are not unique, we introduced an extra invariant to make sure that keys with the same priority are extracted in key order. This requires additional comparisons in various performance-critical functions.

  • Remove the extra invariant.
  • Remove the extra code to preserve it.
  • Change (==) to work in key order instead of priority order. Keys are unique, so this is very natural.
  • Remove a bunch of now-redundant constraints.

@treeowl treeowl force-pushed the no-beats branch 3 times, most recently from 8fefbb8 to 2d971b8 Compare May 16, 2023 03:58
We used to define `==` to test equality in *priority* order. This was
expensive in and of itself, since it takes logarithmic time to produce
each entry. But the situation was worse than that: since priorities are
not unique, we introduced an extra invariant to make sure that keys with
the same priority are extracted in key order. This requires additional
comparisons in various performance-critical functions.

* Remove the extra invariant.
* Remove the extra code to preserve it.
* Change `(==)` to work in key order instead of priority order.
  Keys are unique, so this is very natural.
* Remove a bunch of now-redundant constraints.
* Document the newly introduced nondeterminism.
@treeowl
Copy link
Contributor Author

treeowl commented May 16, 2023

@jaspervdj, do you want to go this way? If not, I guess I can open a PR that just changes the Eq instance. While the benefits won't be as great, there seems to be no possible reason not to at least do that.

@jaspervdj
Copy link
Owner

Thanks for looking into this @treeowl! I am definitely fine changing the Eq instance.

I'm a bit more worried about changing the minView behaviour. However, I read through the documentation and it seems like we never documented the order of extraction in case priorities are equal, so probably it's fine? We probably need to bump the package to 0.3.0.0 just to make sure, which is a bit annoying for consumers.

Do you have any benchmark data laying around? What sort of speedups are we talking about?

@treeowl
Copy link
Contributor Author

treeowl commented May 17, 2023

No, I don't have data, but I can try to get some.

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