Add a min-max heap implementation that can be used to back a priority queue

[AquaGeek]

Description

This is an extraction of the heap implementation hammered out in #51.

Detailed Design

One of the big advantages of using a min-max heap is that we don't need to bifurcate into a min heap or max heap and keep track of which kind is used — you can pop min or max, as needed. This makes it very useful as the backing storage for a double-ended priority queue.

public struct Heap<Element: Comparable> {
    public var isEmpty: Bool
    public var count: Int

    // Read-only view into the underlying array
    public var unordered: [Element]

    // Initializers
    public init()
    public init<S: Sequence>(_ elements: S)

    // Mutations
    public mutating func insert(_ element: Element)
    public mutating func insert<S: Sequence>(contentsOf: S)
    public func min() -> Element?
    public func max() -> Element?
    public mutating func popMin() -> Element?
    public mutating func popMax() -> Element?
    public mutating func removeMin() -> Element
    public mutating func removeMax() -> Element
}

// Array literal
extension Heap: ExpressibleByArrayLiteral {
    public init(arrayLiteral elements: Element...)
}

// Iteration
extension Heap {
    public struct Iterator: Sequence, IteratorProtocol {}

    public var ascending: Iterator
    public var descending: Iterator
}

Future Directions

• Add replaceMin and replaceMax (@timvermeulen has a branch of this already)
• Implement a priority queue on top of this (that work will be picked back up in Add a priority queue implementation built on top of Heap #51)

Documentation

The public APIs have largely been documented. An overview document has been added to the Documentation directory that is adapted from the API review and code comments.

Testing

There are unit tests for the added Heap type.

Performance

Performance tests have been added (including comparable tests against a CFBinaryHeap). We may want to revisit the library JSON to ensure we have the desired comparisons defined. (Can somebody with a better understanding of how all of that works/is structured take a look? Maybe @lorentey?)

Source Impact

This is purely additive. No existing APIs were changed, deprecated, or removed.

Checklist

• I've read the Contribution Guidelines
• My contributions are licensed under the Swift license.
• I've followed the coding style of the rest of the project.
• I've added tests covering all new code paths my change adds to the project (to the extent possible).
• I've added benchmarks covering new functionality (if appropriate).
• I've verified that my change does not break any existing tests or introduce unexpected benchmark regressions.
• I've updated the documentation (if appropriate).

[AquaGeek]

OK, Heap.md has been added to the Documentation directory. This should be ready for a full review.

[AquaGeek]

I've updated the CFBinaryHeap benchmark. Using raw integers instead of NSNumbers significantly altered the performance comparison. Beyond ~256 elements, CFBinaryHeap is faster than Heap for both popMin and popMax.

[hassila]

I've updated the CFBinaryHeap benchmark. Using raw integers instead of NSNumbers significantly altered the performance comparison. Beyond ~256 elements, CFBinaryHeap is faster than Heap for both popMin and popMax.

Thanks a lot, interesting!

I’d just want to (very shortly) make the point here that it’s always faster, CF only has a higher constant creation time (I know you know, but it was related to the point of how to display performance numbers in the related PR) - this conflation of numbers leads to unprecise characterizations of perf at times. Anyway, I’ll be silent about that now :-).

[AquaGeek]

@kylemacomber How do you want to handle this? The code was already mostly reviewed in the other PR; should @timvermeulen take another look?

Also, do we want to merge this into main or into a priority-queue branch?

[kylemacomber]

@lorentey is the final boss 😉

[AquaGeek]

@lorentey Any update on when you might be able to look at this? 🙏

[lorentey]

I'm on it! It's slow going, but I'll submit the review soon! (Things are looking good.)

[lorentey]

Quick update on our plans: I'd like to release version 1.0 of this package relatively soon with the existing three types (to signal that we're committing to source stability), then quickly follow it up with version 1.1 that includes this work as well as anything else that's ready.

However, we don't need to wait on the 1.0 tag to merge new work to main -- we can just tag 1.0 from a release branch, like we do for Swift releases. So we can land this as soon as I get my act together and finish the review. :-)

[lorentey]

I think this is ready to land, with a small handful of nits that would be useful to resolve before merging.

I also noted several larger FIXMEs/experiments that we should eventually get around to implementing, but they need not be done before merging this PR.

[lorentey]

FIXME: I wonder if objc_msg_send introduces measurable overhead in the benchmarks -- it's fast, but I don't expect it fares well when compared to a direct function call, and these microbenchmarks are tiny enough that even minute overheads have an effect. It would be interesting to compare results to a variant where these are all C functions.

(This doesn't need to be resolved before this lands.)

[AquaGeek]

Yeah. I started writing a C++ benchmark for comparison but haven't been able to finish that yet. I'd be happy for any assistance improving the comparative benchmarks here from anyone; I'm quickly approaching the limits of my understanding of perf.

[lorentey]

I can have a go at it relatively soon.

[lorentey]

FIXME: IIRC this has a cost of a ~multiplication operation; it may be better to explicitly keep track of the depth of an Index, by switching to a custom struct rather than just a naked Int value.

This would also make the parent/left/right child operations more elegant:

struct _Index: Comparable {
  let offset: Int
  let level: Int

  func parent() -> _Index?
  func grandParent() -> Index?
  func leftChild() -> _Index
  func rightChild() -> _Index
}

Given that this is not going to be exposed as public, it may make sense to use UInts to (trivially) speed up operations like the division by two in parent().

This doesn't need to be addressed before landing this.

[lorentey]

Issue filed as #71

[lorentey]

Nit: why not just call _storage.swapAt(_:_:) here?

[AquaGeek]

It was determined in the original PR that doing it this way was more performant.

[lorentey]

Wow, that's terrible. This is another FIXME for later -- there is no reason Array.swapAt needs to be any slower than doing this.

[lorentey]

Issue filed as #72 (in addition to the existing Swift issue https://bugs.swift.org/browse/SR-14778 -- thanks very much for filing that!)

[lorentey]

FIXME: Combinatorial tests would work great on these!

This doesn't need to be addressed before this lands.

[lorentey]

Issue filed as #73

[lorentey]

This is useful information to have in an "Implementation Details" section, but it doesn't tell the reader what a heap is and why they would want to use it. We should add an introduction that is useful for a wider audience.

Something like this would be a good start:

A collection of comparable items arranged in a way that allows efficient access to its minimal and maximal elements.

Heaps can be used to efficiently provide sorted access to items that arrive incrementally and out of order. For example, a server application may use a heap to store its queue of incoming client transactions, processing them in order of decreasing priority.

Heaps provide highly optimized implementations for the following operations:

• insert(_:): Inserts a new element in O(log(count)) time.
• min()/max(): Retrieve (but does not remove) the minimum/maximum item in O(1) time.
• removeMin()/removeMax(): Remove and return the minimum/maximum item in O(log(count)) time.

<Some toy sample code illustrating these operations>

[lorentey]

Issue filed as #74

[AquaGeek]

OK, updated things. I think the only things left from the "need to fix before merging this" bucket are the documentation changes.

[lorentey]

The doc updates we can also do as followup work. Let's land this! 🚀

Do you need me to help resolve the conflicts?

[AquaGeek]

The doc updates we can also do as followup work. Let's land this! 🚀

Sounds good!

Do you need me to help resolve the conflicts?

I can do it. Do you want me to rebase on main or pull it in to resolve the conflicts? I'm going to rebase.

[AquaGeek]

Merge conflicts have been resolved. We should be good to go! 🚀

Do you want me to create GitHub issues for the follow-up changes?

[lorentey]

@swift-ci test

[lorentey]

Do you want me to create GitHub issues for the follow-up changes?

Oh, that's a good idea -- let me do that though; it seems unfair to ask you to file issues for my loosely explained FIXMEs. 🙈

[lorentey]

19:05:20 /home/buildnode/jenkins/workspace/pr-swift-collections-linux/branch-main/swift-collections/Benchmarks/CppBenchmarks/src/BinaryHeap.m:12:9: fatal error: 'Foundation/Foundation.h' file not found
19:05:20 #import <Foundation/Foundation.h>
19:05:20         ^~~~~~~~~~~~~~~~~~~~~~~~~
19:05:20 1 error generated.

Ah, this makes sense -- I think we'll need to add #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS) around these benchmarks.

(Which is super annoying, as it makes the benchmark library json platform dependent. Argh -- this is another FIXME for later.)

[lorentey]

OK, I filed issues for all my FIXMEs:

https://github.com/apple/swift-collections/issues?q=is%3Aissue+is%3Aopen+label%3AHeap

As usual with these, some of them are suggestions for optimizations that may turn out to be unnecessary or even harmful. The benchmarks will be the judge. 😝

[lorentey]

@swift-ci test