-
Notifications
You must be signed in to change notification settings - Fork 37
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 term::relation_with() #59
Conversation
At a first glance, it seems to be a non-negligible readability/performance tradeoff. Would you mind waiting a bit for those kind of perf improvements? It also would complicate the promising |
Yes (cc #49) it would need to be added to the proposed trate. We could provide a default implementation that uses
Yes, we can definitely weight. Lets get 0.2 out. |
e1bd299
to
407c58e
Compare
407c58e
to
63b325d
Compare
Using the elm benchmark this was a smaller improvement then the noize. I.E. Criterion thought this was faster but not statistically significantly so. So this should be put on the back burner for a while. When we find other ways to make the elm benchmark faster, this may pull its weight. |
e7d84bf
to
4c05fbf
Compare
running on top of #74 using the benchmarks from #75
So it now looks like an improvement. Can the code be cleaned up to make it worth it? It needs work, but we have something to talk about. |
I'm personally still not very attracted by this change. Thanks to all other changes, the perf improvement here is down to 9% where it was roughly 20% at the beginning. And this makes the code more complex to read and to explain, it also doesn't match anymore with the expression in the guide documentation. Let's keep it around and focus on features important for cargo instead. Once we have something working for cargo, we can see if that perf improvement is still worth the change. |
7342ce5
to
8375f23
Compare
I am closing, thanks to |
In #157 (comment) reported large improvements from reducing the calls to |
In #27 it was measured that we spend a lot of our runtime in
intersection
s inrelation_with
. For that use case we do not need the exact list of what versions are in theintersection
just a trinary state. So this adds a function toRange
to compute that directly.Criterion thinks this is a ~20% improvement in runtime of the benchmarks from #34.
This is mostly because of skipping the cache miss and allocation of making a new
Range
. But also because we can short circuit when we see aRelation::Inconclusive
.This still needs wordsmithing.