[Heap] Performance tweaks

[lorentey]

• @inline(__always) has a detrimental effect when used on large functions. Let's not do that.
• Speed up invariant checking with COLLECTIONS_INTERNAL_CHECKS (resolves [Heap] Improved consistency checking #69)
• Replace naked ints with _Node, precalculating levels for each offset (resolves [Heap] Switch to a custom _Index type that incrementally tracks depth #71)
• Rebase heap operations on UMBP instead Array, eliminating unnecessary index validation (resolves [Heap] Express heap operations on UnsafeMutableBufferPointer #75)
• Add some (slightly questionable) effects attributes to heap internals.
• Optimize removals and heap construction

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 (if appropriate).
• I've added benchmarks covering new functionality (if appropriate).
• I've verified that my change does not break any existing tests or introduce unexplained benchmark regressions.
• I've updated the documentation if necessary.

[lorentey]

@swift-ci test

[AquaGeek]

I'd be interested to see the performance difference here.

[lorentey]

Removing @inline(__always) turned out to be a significant regression. 😅This won't land as is.

I'd like to keep an eye on code size though -- always inlining large(ish) functions is generally a bad move. We'll see if followup work can make some of this work better.

[lorentey]

The thing I'm worried about is that the sequence-based initializer, insertions, removals are currently all emitted as large monolithic functions, with no code sharing between them.

They aren't terribly large (e.g., the sequence initializer is 1200 bytes on x86_64), but they get duplicated with each specialization, and this can add up.

If we can arrange things so that the bubbleUp/Down function calls remain intact without ruining performance, then that would calm my nerves a bit. :-)

[lorentey]

@swift-ci test

[lorentey]

Benchmark progression:

• cc69069: Original
• 891eb7a: _Node
• 6822d83: _UnsafeHandle
• 454da8f: @inline(__always) → @_effects(releasenone)

[lorentey]

@swift-ci test

[lorentey]

This gets us to ~parity with std::priority_queue, even slightly beating it on large heaps. We still have a slight disadvantage at small sizes, though. (swiftlang/swift#38867 might help close that gap.)

[lorentey]

Cc @AquaGeek

Most of these are dirty tricks that reduce code readability (with the possible exception of _Node), but they are probably worth it for the performance boost.

The addition of the @_effects attributes isn't entirely justified; if we have to take those out, then that will impact insert quite a bit. 🤔

[hassila]

Looks like interesting progress - just one thought, wouldn’t it make sense to use e.g https://github.com/skarupke/heap as the reference for the heap performance rather than the std-prioq? (And compare the actual PriorityQueue with the std-prioq).

[xwu]

Seems 891eb7a: _Node is a straight-up win in performance over the original, and the rest speed up some and regress some here and there.

[lorentey]

just one thought, wouldn’t it make sense to use e.g https://github.com/skarupke/heap as the reference for the heap performance rather than the std-prioq? (And compare the actual PriorityQueue with the std-prioq).

std::priority_queue implements a heap, closely matching our Heap (although one-sided) -- I think the comparison against it is fair (although these are never really apple-to-apples comparisons). The PriorityQueue concept doesn't directly exist in the STL, although (depending on the internal impl) I expect std::priority_queue with a custom comparator would still serve as a useful baseline for it.

std::priority_queue being one-sided does not make it a bad baseline -- it solves a simpler problem than Heap, but our hypothesis was that min-max heaps will provide matching or better performance anyway.

It would be interesting to see benchmarks against @skarupke's implementation! I'll see if I have time to run some experiments over the weekend. 🤔 We won't add those benchmarks to this repository though.

Another interesting source of data structure implementations would be LLVM's ADT library.

I don't think we need to add an extensive library of reference benchmarks though -- verifying against some plausible baseline is useful during the initial implementation effort, but afterwards I believe we should be primarily comparing potential changes against earlier versions of this package, not against other languages.

[lorentey]

One thing that we still need to address is that the code does not currently implement the optimization techniques that make https://github.com/skarupke/heap fast. We need to reorder comparisons in the grandchild min/max search, and use a more clever heapify loop.

[lorentey]

The results above were collected on a 5-years old MacBook Pro. M1 results below!

The insert benchmark results are significantly different on M1 -- for insertions on M1, 6822d83 was an even more significant regression, not an improvement. This may be related to swiftlang/swift#38867, although I expected swift_retain/relase would be less costly on the M1, not more. More analysis is needed.

Versus baselines:

[karwa]

Drive-by comment:

Have you tried using a ContiguousArray for Heap._storage? It may help reduce code size, as regular Array needs to account for potential bridged storage. There may be places where the compiler can’t prove it isn’t bridged.

If you need it as an Array elsewhere, constructing an Array from a ContiguousArray is basically free. It just retains the existing storage, it doesn’t copy.

[lorentey]

Oh yeah, that's a good suggestion; OrderedSet does the same. It shouldn't* have an effect on the current benchmarks, as Element is always Int and Array<Int> is statically known to never be verbatim-bridged -- but I expect there would be a benefit for class-keyed heaps.

* Although ContinuousArray sometimes misses some of the optimization love that Array gets, so it could be slower sometimes -- for instance, I forgot to apply swiftlang/swift#38867 to ContiguousArray just now. 🤦

[lorentey]

c4282ca improves popMin/popMax (and the sequence initializer) by reviewing trickleDown and optimizing things:

• Replace swapAt with a scheme where we keep a hole in the storage buffer
• Slightly shorten min/max dependency chain in primary sink loop

In exchange, we get even less readable code.

M1 benchmarks:

[lorentey]

@swift-ci test

[lorentey]

OK, I'm happy with where we are now. I think insert could probably be improved if we could maintain a hole in the storage array rather than swapping things in it -- but that would need an append variant of Array.init(uninitializedCapacity:initializingWith:) so that's for later.

Final measurements with a toolchain that includes swiftlang/swift#38867 & swiftlang/swift#38898: (taken on a Mac Pro this time)

[lorentey]

@swift-ci test

[compnerd]

This seems to have regressed the Windows build: https://dev.azure.com/compnerd/3133d6ab-80a8-4996-ac4f-03df25cd3224/_apis/build/builds/53542/logs/239