-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
proposal: container/heap: add Heap, a heap backed by a slice #47632
Comments
I'm worried about doing too much in the standard library in Go 1.18. |
I don't think this is going to be the right path for generic data types. I think it is better to initialize the heap with a comparison function. My argument is that if you have a comparison function, it's tedious to create a type with a I admit that this means that the zero value can't be used directly. Still, I think that overall it is the more flexible approach. |
The docs should mention what happens for I'm not quite seeing what |
Done. |
The playground example I linked is an adapted version of the PriorityQueue example from the In general, I would expect it to be used to associate each element with its index somehow, whether that's as part of the element (as in my example) or using some secondary data structure such as a
I don't follow this. |
If the secondary data structure is another slice, then, you're right, we need both the old and the new index. |
This proposal has been added to the active column of the proposals project |
@ianlancetaylor yes, an auxiliary parallel slice that mirrors all the swaps of the I'm not sure why you'd ever want to maintain a parallel slice, though. If you have some extra data to associate with each heap element, why not add it to the element itself? |
OK, after some thought, I agree that this will make many use cases simpler. I updated the proposal to use a |
I'm also in favor of less being a function passed into the constructor. In my usages, I often use heap with a local type (inside the function) which cannot have a method. The reverse is possible: a type with a less method can pass the method value to the constructor. |
It sounds like we should put this on hold for after Go 1.18. Any objections to that? |
another possibility:
|
I'm generally in favor of exporting Furthermore, one advantage of the original |
Placed on hold. |
Over at #47619 (comment) @anacrolix asked about a concrete implementation of a generic slice-based heap. In my original proposal here, I linked to a very quick'n'dirty one in the playground (that simply wraps container/heap's current interface), but it would be interesting to do a proper implementation and compare some benchmarks to see how the type-parameterized version compares to the |
If you do that, please measure the code segment size too. I'm curious to know how much expansion to expect from multiple instantiations of a generic type. In C++ it was occasionally worth defining a widely used template as a mere wrapper around an untyped generic implementation, to reduce the quantity of code duplicated for each new template instantiation. The GNU linker was smart enough to coalesce templates that were essentially identical (e.g. vector of X* vs Y*). I don't know what the Go toolchain's notion of equivalence is, and presumably it varies across implementations. |
It may be a bit awkward to have Pop and Peek to panic if heap is empty, since now caller needs to always keep track of the number of elements in the heap. Maybe, use the func (h *Heap[E]) Pop() (E, bool)
func (h *Heap[E]) Peek() (E, bool) Google's generic BTree implementation uses this approach. As for the PS. @rsc, since 1.18 is long out, maybe this issue can be unheld? |
@SaveTheRbtz I don't think you have to keep track. You only have to call |
Well, not exactly "keep track"; they just need to call Len. With the proposed signature, typical Heap-using code looks like this:
With the boolean result, the loop looks like:
This particular case doesn't seem like much of an improvement. I'm sympathetic to the notion that |
I had a need to play around with this, so I did an implementation and wrote some code generation to compare generic types which you can see here: https://go.dev/play/p/aQWbwOAnfXI attached is resultant generated source code and the output of cmdgen_main.go.txt |
A) This proposal can probably come off of hold now. B) I would like to see this implemented as container/heap/v2 with this api: package heap
func New[E any](cmp func(E, E) int) *Heap[E] // Instead of a less function
func (h *Heap[E]) All() iter.Seq2[int, E] // No Slice() method
// Other methods as proposed above. |
I have been using this form: https://github.com/anacrolix/generics/blob/main/heap/slice.go#L46-L51. It tries to avoid the heap allocation of |
That's interesting. One idea would be to add heap.Slice[E any] to heap v1 and keep using the old interface functions, and also add a new heap v2 package that hides the slice and does its own memory management. |
I suggest changing the proposed implementation of the func (s *sliceHeap[E]) Pop() interface{} {
var zero E
e := s.s[len(s.s)-1]
if s.setIndex != nil {
s.setIndex(e, -1)
}
// avoid memory leak by clearing out popped value in slice
s.s[len(s.s)-1] = zero
s.s = s.s[:len(s.s)-1]
return e
}
|
But we already have a heap implementation that doesnt use generics... So we are stuck with that implementation until some "best" practice API is found. Specifically for something like containers that may be very opinionated. I think this poses a lot of hesitation which is not necessarily a bad thing, and it is important to note any feature added to Go should be considered forever. However with the introduction of the slice package it seems hypocritical to not do it for also containers at this point. Given that we are much past Go 1.18, we should really relook at this. And we could always add this to the experimental package until we get it right. My hope was that this was not implemented due to the amount of features and content within v1.18 and nothing more. I suggest removing the "proposal hold" |
What are thoughts to give a fixed priority for each element at its input into the
The A heap relies on comparisons being both |
I believe in every case I've needed a heap so far, the |
@earthboundkid regarding this suggestion:
|
@rossb83 / @Merovius: I disagree with replacing the comparison function with a priority In the use cases I've studied, it's very common that there is no single priority number and the comparison function compares multiple attributes over a sequence of tie-breaking steps. Here's a simple example:
This one just has two comparisons, but I have other examples that have 4 or more. Turning that kind of thing into a single comparison integer (for example) is annoying to write and error-prone (remember to consider overflow). By contrast, if you already have a simple priority integer, the
I don't really follow this objection. I've reviewed a bunch of uses of container/heap and I don't recall anything like this being an issue. |
Can we come back to this? What do we need to get this into the standard library? A priority queue is a pretty common tool to have in standard libs. I believe the proposed interface looks like it would fit 90% of use cases and save devs a lot of work. |
Second this, I would love to see this targeted for the next major go release. |
This proposal is for use with #43651. I propose to add a parameterized
Heap
type to thecontainer/heap
package. For the typical usage ofcontainer/heap
where the heap is backed by a slice, usingheap.Heap
is safer, more convenient, and more readable:any
are required.Less
,Len
,Swap
,Push
, andPop
), most of which are typically boilerplate, only thecmp
function is needed.Push
,Pop
, etc. be methods is more natural than functions:pq.Push(e)
vs.heap.Push(pq, e)
.Update 2024-07-22: Switched from a
less
function to acmp
function.Update 2021-08-28: Removed the Lesser interface; pass in a
less
function to the constructor.Notes
I evaluated the uses of container/heap in our company's internal code as well as my own projects and found that all of them used a heap backed by a slice. This indicates that a slice-based implementation could save a good deal of boilerplate in much the same way that
sort.Slice
saves boilerplate oversort.Sort
.A simpler API would do away with the "slice"-ness, leaving the implementation unspecified and removing the index-based methods:
However, my review of
container/heap
usage showed that this simplifiedHeap
would cover fewer than half of those real-life use cases. Many real uses ofcontainer/heap
need the index-based methodsFix
orRemove
; for instance, to delete items from a priority queue.A different simplification would be to use
constraints.Ordered
elements rather than theLesser
constraint. While this makes for concise example code, in practice, such heaps seem relatively rare; all the uses I analyzed had slice elements of some compound type.Here is a playground implementation of the proposal showing the priority queue example implemented using
Heap
.The text was updated successfully, but these errors were encountered: