Skip to content

Conversation

@thestinger
Copy link
Contributor

This adds a priority queue implemented as a binary heap to std. The API is heavily inspired by python's heapq module except that it's encapsulated in a container and uses a max-heap to get the default of high values as the max priority and sorting in ascending order.

The naming (priority_queue::PriorityQueue) is quite verbose, so it could be heapq::HeapQ instead if that's preferred.

@erickt
Copy link
Contributor

erickt commented Dec 11, 2012

That looks pretty good. I'm not sure if you need the copy constructor though. I think the only function that require it are top and maybe_top. I think you could rewrite to:

    pure fn top(&self) -> &self/T { &self.data[0] }

    pure fn maybe_top(&self) -> Option<&self/T> {
        if self.is_empty() { None } else { Some(self.top()) }
    }

@thestinger
Copy link
Contributor Author

@erickt: the copies the almost gone now, but there are 4 unnecessary ones left to get rid of in the siftdown/siftup implementations. There are also two errors from moving out of self but it looks like that should be fixed soon, which is why @pcwalton suggested making them methods.

@thestinger
Copy link
Contributor Author

I can see two ways of getting rid of the remaining copies:

A) using unsafe blocks, and moving around 1 junk location
B) using swaps, which would seemingly almost double the number of moves needed

After that, the only thing keeping the Copy trait as a requirement is not being able to move out of self at the moment.

@erickt
Copy link
Contributor

erickt commented Dec 14, 2012

@thestinger: As far as I know, moves don't have any overhead, they are just for tracking who owns the pointers. So I suggest using swaps.

@erickt
Copy link
Contributor

erickt commented Dec 14, 2012

Also, @pcwalton just landed a patch that lets you move out of self in cd12073, so you should be all set now for removing the copy kind.

@thestinger
Copy link
Contributor Author

After the conversations about this on IRC and some testing, I decided to go with using unsafe blocks. I think it's worth using it for a data structure that will likely be fairly widely used in algorithms where performance is critical. For some data types (strings), the comparisons dominate the time - but for some the difference between compare + move and compare + swap is pretty big.

I documented the reasoning behind doing it in a comment to make it clear why it's safe, and why it was done for anyone looking at the code in the future.

@thestinger
Copy link
Contributor Author

Here's the diff for the alternate implementation without unsafe (using swaps), for the record: https://gist.github.com/4299825

@brson
Copy link
Contributor

brson commented Dec 17, 2012

Merged. Thanks!

@brson brson closed this Dec 17, 2012
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.

3 participants