-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
PriorityQueue: use O(logN) algorithm for dequeue!(pq, key) #8011
Conversation
Just a speedup of 10^4, huh? I think this is great and 0.4 is the time to break stuff, right? (particularly for big speedups). |
Yeah, I'd say go for it. Put the change in NEWS and describe how it breaks. |
PriorityQueue: use O(logN) algorithm for dequeue!(pq, key)
@quinnj, yes, I'm hoping to use this as my submission for the next contestant round on AmericanPerformanceIdol. |
LoL @ AmericanPerformanceIdol |
Documentation done. Thanks for looking! |
A thought for smaller breaking changes like this: if its not 100% obvious how to maintain 0.3 and 0.4 compatibility, it would be useful to provide a snippet showing how to do so |
To harp on one of my usual themes, it would have been better to do the optimization and adding the parameter in separate commits. Since it's early in 0.4 I think we'll be able to keep all of this though. |
Will keep that in mind. Thanks for the reminder. |
This PR should speed up some of my own work by a factor of 10^4 or so. The main highlight is switching from an
O(N)
algorithm to one that'sO(logN)
indequeue!(pq, key)
. That change should not be controversial.However, I made a couple of other changes that might merit some discussion:
o::Ordering
, butOrdering
is an abstract type. Consequently calls that involvedlt
had to use generic fallback paths, and allocated lots of memory. I added the specific ordering type to the parameters. However, this is breaking since anyone who formerly wrotepq = PriorityQueue{K,V}()
will have to change it. Because it's quite irritating to have to writepq = PriorityQueue{K,V,typeof(Base.Order.Forward)}()
, I added the following constructor variant:pq = PriorityQueue(K,V)
. I am a little worried this might cause trouble in ways I'm not thinking of. The performance enhancement from this change is not as huge at the change in thedequeue(pq, key)
algorithm (as you'll see below), but it's still a factor of 2 or so and it affects basically all operations with PriorityQueues. So to me it seems worth it.Pair
for storing the key, value pairs.Here are some benchmark results using a version identical to what I checked in here, but outside of
Base
so that it allows direct comparison in the same session:Results:
Since my own problems are more likely to be in the
10^6-10^7
range, the actual expected benefits are of course much larger.