Persistent collections updates (part 8) #184

lorentey · 2022-09-23T19:04:30Z

Implement logarithmic-time index(_:offsetBy:) and index(_:offsetBy:limitedBy:) operations.
Cache the shared hash value of items in each collision node. (At the front of the storage buffer, where child entries would otherwise be stored.)
Review and sanitize/simplify node allocation logic. Ditto for key-based lookups.
Implement better handling of compressed paths in isSubset(of:)/isDisjoint(with:)

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.

…tself Also, don’t check `DefaultIndices` — it’s fine to assume it implements Collection as long as the base collection is correct.

…ex(_:offsetBy:limitedBy:)

…ild in a non-unique node Oops, this did not account for reversed item order, so it was spawning the new child with the wrong item.

This will simplify merge operations as we no longer have to spend effort on preventing repeated rehashings of items within collision nodes. Also, review and simplify node allocation and lookup code.

…ubset/isDisjoint

lorentey · 2022-09-23T19:28:09Z

@swift-ci test

lorentey · 2022-09-23T20:17:15Z

Showing off the algorithmic improvement by striding over the collection in 10 equal steps using index(_:offsetBy:):

That's 2066x faster on average. 🤓

The new executable target `memory-benchmark` collects some memory usage statistics and outputs average memory efficiency for a range of sizes in CSV format. Bump required toolchain version for benchmarks to Swift 5.5.

lorentey · 2022-09-24T00:02:11Z

The last commit adds a very rudimentary memory benchmark, as a standalone executable target in the hidden benchmarking package. It produces CSV output, comparing the memory efficiency of the standard Dictionary vs PersistentDictionary. Current (untuned) results are:

This plots memory efficiency as a function of collection size. Higher values are better.

I define "memory efficiency" as the net amount of memory needed to store the items in the collection divided by the gross amount of memory allocated overall. This ratio generally depends on the memory layout of the Key/Value types -- smaller payloads generally lead to lower memory efficiency, especially for tree-based collections. (As the relative weight of node headers + child references will be higher.)

For Dictionary<String, String>, average memory efficiency (over a logarithmic scale of sizes from 1 to 1M) is about 55%, while for PersistentSet<String, String> we currently measure about 54% -- this is pretty good, considering we haven't really tuned the allocation patterns yet. (The HAMT is really efficient for representing small dictionaries, because of the node compression scheme -- this small-scale behavior is why it is able to catch up to the standard Dictionary. The log scale gives weight to small values, which would be a bit unfair, except it tends to match actual usage -- small data sets are more frequent.)

Of course, this is measuring a single collection instance -- the memory efficiency of PersistentSet goes through the roof in use cases that actually make use of persistency. (Mutated copies of the same original dictionary share their storage nodes, making them extremely memory efficient.)

lorentey · 2022-09-24T04:33:56Z

d87f5fd replaces recursion with looping for simple key-based lookups, resulting in a really nice 47-59% speedup.

Tasks with difference scores larger than 1.05:
  Score   Sum     Improvements Regressions  Name
  1.595   1.595   1.595(#75)   0.9998(#1)   PersistentDictionary<Int, Int> defaulted subscript, successful lookups (*)
  1.584   1.584   1.584(#76)   1.000(#0)    PersistentDictionary<Int, Int> subscript, successful lookups (*)
  1.486   1.486   1.486(#75)   0.9997(#1)   PersistentDictionary<Int, Int> defaulted subscript, unsuccessful lookups (*)
  1.474   1.474   1.476(#75)   0.9984(#1)   PersistentDictionary<Int, Int> subscript, unsuccessful lookups (*)

…StringConvertible, CustomReflectable conformances

… (untested) This adds an experimental _FastMembershipCheckable protocol to speed up these relationship predicates when possible — having separate overloads for each known collection with a fast containment check is not going to scale for long. This commit also introduces a set of isEqual(to:) methods that generalize ==.

Plus add tests covering these as well and mapValues.

…ring (untested)

…ns (untested)

…istentSet and a value transform

lorentey · 2022-09-25T00:22:46Z

Latest commits:

PersistentSet, PersistentDictionary: Add more protocol conformances
PersistentSet: Add isEqual/isSubset/isSuperset/isStrictSubset/isStrictSuperset/isDisjoint overloads
_FastMembershipCheckable: New utility protocol to make the overload set more manageable
PersistentDictionary: Add structural filter and compactMapValues operations
PersistentSet: implement structural filtering, subtraction, intersection
PersistentDictionary: Add an initializer to quickly convert a PersistentSet into a dictionary (without rehashing anything)

Note: The structural filtering/intersection/subtraction operations do not preserve unchanged nodes yet.

lorentey · 2022-09-25T00:24:10Z

It is of course also possible to implement structural union and symmetricDifference operations (as well as structural dictionary merges) -- this is still tbd.

lorentey · 2022-09-25T00:24:31Z

@swift-ci test

msteindorfer

LGTM

msteindorfer · 2022-09-26T13:59:39Z

Sources/PersistentCollections/Node/_Node+Lookups.swift

  internal func get(_ level: _Level, _ key: Key, _ hash: _Hash) -> Value? {
-    read {
-      guard let r = $0.find(level, key, hash) else { return nil }
-      if r.descend {
-        return $0[child: r.slot].get(level.descend(), key, hash)
+    var node = unmanaged
+    var level = level
+    while true {
+      let r = UnsafeHandle.read(node) { $0.find(level, key, hash) }
+      guard let r = r else {
+        return nil
      }
-      return $0[item: r.slot].value
+      guard r.descend else {
+        return UnsafeHandle.read(node) { $0[item: r.slot].value }
+      }
+      node = node.unmanagedChild(at: r.slot)
+      level = level.descend()
    }
  }


It's somewhat surprising just how much of an improvement this turned out to be. IIRC the recursive variant didn't have any ARC traffic, either, so the difference must be largely due to stack overhead. I'm happy to take it though!

msteindorfer · 2022-09-26T14:00:03Z

Sources/PersistentCollections/Node/_Node+Lookups.swift

  internal func containsKey(
    _ level: _Level, _ key: Key, _ hash: _Hash
  ) -> Bool {
-    read { $0.containsKey(level, key, hash) }
-  }
-}
-
-extension _Node.UnsafeHandle {
-  @inlinable
-  internal func containsKey(
-    _ level: _Level, _ key: Key, _ hash: _Hash
-  ) -> Bool {
-    guard let r = find(level, key, hash) else { return false }
-    guard r.descend else { return true }
-    return self[child: r.slot].containsKey(level.descend(), key, hash)
+    var node = unmanaged
+    var level = level
+    while true {
+      let r = UnsafeHandle.read(node) { $0.find(level, key, hash) }
+      guard let r = r else { return false }
+      guard r.descend else { return true }
+      node = node.unmanagedChild(at: r.slot)
+      level = level.descend()
+    }
  }


msteindorfer · 2022-09-26T14:17:44Z

Sources/PersistentCollections/Node/_Node+Builder.swift

+extension _Node {
+  @usableFromInline
+  @frozen
+  internal enum Builder {


msteindorfer · 2022-09-26T15:27:38Z

Sources/PersistentCollections/Node/_Node+Structural filter.swift

+    _ hashPrefix: _Hash,
+    _ isIncluded: (Element) throws -> Bool
+  ) rethrows -> Builder {
+    // FIXME: Consider preserving `self` when nothing needs to be removed.


You should be able to check sub-tree sizes. If the builder result would have the same size as self, you can return self.

That's a great shortcut -- I may just take it!

I'm hoping we can eventually prevent building the result when we'd be throwing it away. It feels like something that should be possible to express without too much additional complexity.

The same shortcut also applies to the other operations that make use of the builder abstraction.

lorentey added 8 commits September 23, 2022 11:15

[PersistentCollections] Remove dead code

8c9ae08

[PersistentDictionary] Add index striding benchmark

9407eff

[tests] Delay checking Indices until we’re done with the collection i…

a453335

…tself Also, don’t check `DefaultIndices` — it’s fine to assume it implements Collection as long as the base collection is correct.

[PersistentCollections] Small path-related adjustments

eefef1e

[PersistentCollections] Implement structural index(_:offsetBy:) & ind…

77f205f

…ex(_:offsetBy:limitedBy:)

[PersistentCollections] Fix indexing oversight when spawning a new ch…

287c677

…ild in a non-unique node Oops, this did not account for reversed item order, so it was spawning the new child with the wrong item.

[PersistentCollections] Cache the item hash within collision nodes

9150bbf

This will simplify merge operations as we no longer have to spend effort on preventing repeated rehashings of items within collision nodes. Also, review and simplify node allocation and lookup code.

[PersistentCollections] Slightly optimize compressed path case in isS…

2193e79

…ubset/isDisjoint

lorentey requested a review from msteindorfer September 23, 2022 19:04

[PersistentDictionary] Add a quick&dirty memory benchmark

92d1d12

The new executable target `memory-benchmark` collects some memory usage statistics and outputs average memory efficiency for a range of sizes in CSV format. Bump required toolchain version for benchmarks to Swift 5.5.

[PersistentCollections] Optimize key-based lookup operations

d87f5fd

lorentey added 7 commits September 24, 2022 17:02

[PersistentSet] Add ExpressibleByArrayLiteral, Codable, Custom[Debug]…

00ae83d

…StringConvertible, CustomReflectable conformances

[PersistentDictionary] Add CustomReflectable conformance

1c6e5cb

[PersistentDictionary] Implement structural filter & compactMapValues

826e22c

Plus add tests covering these as well and mapValues.

[PersistentSet] Implement structural subtraction, intersection, filte…

c92b0fa

…ring (untested)

[PersistentSet] Implement (naive) union, symmetricDifference operatio…

590dfeb

…ns (untested)

[PersistentDictionary] Add a structural initializer that takes a Pers…

ba303dc

…istentSet and a value transform

msteindorfer approved these changes Sep 26, 2022

View reviewed changes

lorentey merged commit ac35613 into apple:release/1.1 Sep 26, 2022

lorentey added this to the 1.1.0 milestone Oct 10, 2022

lorentey added the HashTreeCollections Hash-array mapped prefix tree implementations label Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persistent collections updates (part 8) #184

Persistent collections updates (part 8) #184

lorentey commented Sep 23, 2022

lorentey commented Sep 23, 2022

lorentey commented Sep 23, 2022 •

edited

Loading

lorentey commented Sep 24, 2022

lorentey commented Sep 24, 2022

lorentey commented Sep 25, 2022 •

edited

Loading

lorentey commented Sep 25, 2022

lorentey commented Sep 25, 2022

msteindorfer left a comment

msteindorfer Sep 26, 2022

lorentey Sep 26, 2022

msteindorfer Sep 26, 2022

msteindorfer Sep 26, 2022

msteindorfer Sep 26, 2022

lorentey Sep 26, 2022

msteindorfer Sep 26, 2022

Persistent collections updates (part 8) #184

Persistent collections updates (part 8) #184

Conversation

lorentey commented Sep 23, 2022

Checklist

lorentey commented Sep 23, 2022

lorentey commented Sep 23, 2022 • edited Loading

lorentey commented Sep 24, 2022

lorentey commented Sep 24, 2022

lorentey commented Sep 25, 2022 • edited Loading

lorentey commented Sep 25, 2022

lorentey commented Sep 25, 2022

msteindorfer left a comment

Choose a reason for hiding this comment

msteindorfer Sep 26, 2022

Choose a reason for hiding this comment

lorentey Sep 26, 2022

Choose a reason for hiding this comment

msteindorfer Sep 26, 2022

Choose a reason for hiding this comment

msteindorfer Sep 26, 2022

Choose a reason for hiding this comment

msteindorfer Sep 26, 2022

Choose a reason for hiding this comment

lorentey Sep 26, 2022

Choose a reason for hiding this comment

msteindorfer Sep 26, 2022

Choose a reason for hiding this comment

lorentey commented Sep 23, 2022 •

edited

Loading

lorentey commented Sep 25, 2022 •

edited

Loading