Better Sequence support for Heap
#114
Conversation
| /// - Warning: If `Element` is a reference type, do not mutate elements such | ||
| /// that their relative order rankings change. |
There was a problem hiding this comment.
This warning seems out of place here. If it makes sense to add it, it needs to go on the top level Heap type.
| /// | ||
| /// - Warning: If `Element` is a reference type, do not mutate elements such | ||
| /// that their relative order rankings change. | ||
| public typealias UnorderedView = [Element] |
There was a problem hiding this comment.
What's the purpose of this new typealias?
| /// | ||
| /// - Complexity: O(log `count`). | ||
| @inlinable | ||
| public func contains(_ element: Element) -> Bool { |
There was a problem hiding this comment.
Neither of these approaches seem good to me. A contains that requires an allocation doesn't seem like a good idea, especially because it is so easy to navigate the heap -- we can get to parent / left-child / right-child by doing trivial binary arithmetic on the indices.
Let's just implement a depth-first search but limited to strictly O(1) space. (I.e., no recursion.) This can be done because the integer index into the storage index represents a full path in the tree -- we don't need to express it on the call stack (as with recursion), or in an explicitly allocated stack.
The default implementation of contains simply performs a linear search in the storage array; this is effectively a breadth-first search with O(1) space, although it doesn't (can't) implement trimming. That's the algorithm to beat -- it isn't immediately obvious to me that a trimming depth-first search will perform better in practice, at least for element types with a trivial </== implementation.
| /// - Returns: `true` if the element was found in the heap; otherwise, | ||
| /// `false`. | ||
| /// | ||
| /// - Complexity: O(log `count`). |
There was a problem hiding this comment.
| /// - Complexity: O(log `count`). | |
| /// - Complexity: O(`count`). |
| } | ||
|
|
||
| // Out-of-bounds check | ||
| if (isMinLevel ? (<) : (>))(target, nodeElement) { |
There was a problem hiding this comment.
This is clever, but I prefer boring! 😉
| if (isMinLevel ? (<) : (>))(target, nodeElement) { | |
| if isMinLevel ? target < nodeElement : target > nodeElement { |
Better yet, the algorithm could perhaps be "unrolled" to get rid of the isMinLevel branch. (However, it isn't obvious that will actually perform better than this -- this needs to be benchmarked.)
| // Pre-allocate an array for all potential searched-for nodes. (Only | ||
| // appending will happen, no removals nor random inserts, to minimize cache | ||
| // invalidation.) |
There was a problem hiding this comment.
Copying each visited element into a throwaway array is not a good idea.
|
I'm closing this without merging. We decided to get rid of the ordered views -- I was and remain unconvinced that they serve any useful purpose in practice that cannot be better achieved with other means. (They were over-elaborate equivalents of Adding a Let's keep an eye on user feedback once 1.1.0 is out, and evolve |
This request expands on
Sequencesupport forHeaps.underestimatedCount.Heapnow has an element-search method.That last method has two implementations inside, breadth- and depth-first search. Those need to be benchmarked against each other. They also need to be compared against copying an unordered view and running a (linear) search on that. We need various sizes and search target locations (before all actual values, after them, between them, an actual match). The complexity level is a guesstimate; it could be worst-case linear, at least for breath-first. But those are just the time complexities, the scratch space may be significant.
Checklist