-
Notifications
You must be signed in to change notification settings - Fork 31
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: choose package with fewest versions matching (30x speedup) #32
Conversation
.potential_packages() | ||
.next() | ||
.map(|(p, all_terms)| (p.clone(), Term::intersect_all(all_terms.iter()))) | ||
.map(|(p, all_terms)| (p, Term::intersect_all(all_terms.iter()))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a very pricey thing to do if we effectively iterate over all potential packages. In such case, it would probably be worth keeping in memory
not a Vec
of derivations but their intersections directly. I haven't reread the code but I think we may never need individual derivations for anything. Every usage we have of Memory
could be as efficient or more if we actually had the intersection directly.
To be checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, That is definitely worth looking into! But with this already being 30x it is hard to tell based on this benchmark. It seams to worth committing now and evaluating that again when we have a new benchmark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright!
Did you mean 3x speed up then? XD |
No, sorry typo. I ment 0.006. 😄 |
24ac4c6
to
bebd2cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good to me! If that's ok with @aleksator too?, let's merge
I've added a small doc change to the |
This adds the heuristic that is described in the comment. This is the heuristic asked for in #20, but there may be even better ones to try. Unfortunately, it is hard to tell with the existing benchmark as even this makes the benchmark fly. For me the benchmark 0.175 sec -> 0.006 sec!