-
Notifications
You must be signed in to change notification settings - Fork 470
solve for accidental quadratic performance from min max methods #267
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
base: main
Are you sure you want to change the base?
Conversation
4b30cbb to
c4394fb
Compare
| // If we're attempting to prefix more than log n of the collection, it's | ||
| // faster to sort everything. | ||
| guard prefixCount < (self.count / 10) else { | ||
| guard prefixCount <= self.count._binaryLogarithm() else { |
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.
You don't need underscored API for this:
| guard prefixCount <= self.count._binaryLogarithm() else { | |
| guard prefixCount <= (Int.bitWidth - self.count.leadingZeroBitCount) else { |
For a fixed width type like |
| [Soroush Khanlou's research on this matter](https://khanlou.com/2018/12/analyzing-complexity/). | ||
| The total complexity is `O(k log k + nk)`, which will result in a runtime close | ||
| to `O(n)` if *k* is a small amount. If *k* is a large amount (more than 10% of | ||
| to `O(n)` if *k* is a small amount. If *k* is a large amount (more than log n of |
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.
Nit: Your implementation considers anything more than log2(n) to be “large” (not ln or log10); log2 isn’t one of the bases abbreviated “log”.
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.
@xwu Ahh… that's a good point!
TBH… I'm also open to hearing better ideas for how to compute this new threshold. Taking the binary logarithm guarantees our O(n log n) worst-case complexity but is also maybe more conservative than necessary. For an Array of one million elements this only gives us the first 19 elements before we sort the whole Array.
Adding one to the result of the binary logarithm gives us approximately log1.41 for the first 20 elements. We can keep going down that road and adding more… but I don't have a great answer how to analytically solve for what the optimal threshold should be that guarantees our O(n log n) worst-case complexity but keeps working on the "fast path" as much as possible.
|
From those benchmarks it seems like there’s quite a bit of headroom to raise the threshold — are you able to do a bit of experimentation to find where that line actually is? |
https://gist.github.com/vanvoorden/57a2a9768713907677f9fbb258e2c382 @natecook1000 Here are some experiments running different It looks like log1.25 performs better than sorting across the board. It looks like log1.000244140625 performs worse than sorting across the board. Picking something in between like log1.00390625 performs better than sorting on collections of 100K and larger and worse than sorting on collections of 10K and smaller. BTW these are all collections of I'm not sure how else to analytically solve for the "optimal" threshold… any more ideas about that? Is this sort of thing usually found through trial and error and inductive reasoning or more like analytic deductive reasoning? |
There seems to be an accidental quadratic happening in the MixMax methods.1
We document our worst case performance to be
O(n log n). That is our runtime performance if we sort our entire collection of values. Our threshold to sort everything is ten percent of the total count.If we do not sort the entire collection our runtime is
O(k log k + nk). The problem is thatnkcomponent. Ifkis ten percent ofnthis will still grow quadratically with largenand we will not be meeting our promise ofO(n log n)worst case.One option is to change the logic that computes our threshold. Instead of computing a threshold that scales linearly with
nwe can compute a threshold that scales logarithmically withn. This will then help us meet our promise ofO(n log n)worst case.The current implementation presented in this diff uses the underscored
_binaryLogarithmmethod from Standard Library. If we wanted to avoid the underscored API we could also try something similar to what happens inRandomSampleto switch over methods fromDarwinandNumerics.2 Or we could just write our own here in this file.Here are some benchmarks sampled locally on this new threshold:
Checklist
Footnotes
https://forums.swift.org/t/benchmarks-selection-linear-time-order-statistics/83852 ↩
https://github.com/apple/swift-algorithms/blob/1.2.1/Sources/Algorithms/RandomSample.swift#L12-L41 ↩