diff --git a/Sources/MapConductorCore/marker/MarkerGridIndex.swift b/Sources/MapConductorCore/marker/MarkerGridIndex.swift index b381f14..f720f9a 100644 --- a/Sources/MapConductorCore/marker/MarkerGridIndex.swift +++ b/Sources/MapConductorCore/marker/MarkerGridIndex.swift @@ -19,6 +19,27 @@ import Foundation /// the finest one — the first version had a single fixed cell size of 0.005 /// degrees, and a continent-wide box paid for it. /// +/// ## Changes are applied, not rebuilt +/// +/// The first version threw the index away on every change, so a query after +/// any change paid the full build. An app that swaps a few hundred markers on +/// every pan paid it on every tap and every tile; on Android's Pixel 5a that +/// was 83 ms at 60,000 markers and 212 ms at 144,000 after 300 swaps, and a +/// drag paid it every frame. +/// +/// Now a marker that arrives takes the next slot and its entry waits in +/// `pending`; the next query sorts only those and merges them into `packed` in +/// one linear pass. A marker that leaves only empties its slot — queries skip +/// empty slots, and the next merge drops their entries. Only when half the +/// slots are empty, or the index lost track of a marker, does it rebuild from +/// the manager's collection. +/// +/// Finding a leaving marker's entry needs no map. Its entry is in the run of +/// the cell its position falls in, found by binary search and matched by +/// identity. The exception is a marker whose position was changed in place +/// after it was filed — a drag does that — and for that one the slots are +/// scanned, which is still far less than a rebuild. +/// /// Not synchronised. `MarkerManager` holds its lock across every call, and /// taking a second one here would only add cost to a path that is already /// serialised. @@ -44,10 +65,23 @@ final class MarkerGridIndex { private static var nearestLevel: Int64 { 15 } private static var maxNearestRings: Int64 { 100 } + /// Slots the index can hand out before it has to rebuild to renumber them. + private static var maxSlots: Int { 1 << 24 } + + /// Below this many empty slots a rebuild is never worth it. Above it, the + /// index rebuilds once half its slots are empty: the dead entries cost a + /// skip on every query, and the snapshot holds a slot for each. + private static var rebuildMinSlots: Int { 4096 } + private let source: () -> [MarkerEntity] - private var snapshot: [MarkerEntity] = [] - /// Sorted `(mortonKey << indexBits) | position-in-snapshot`. + /// Slot -> marker. A marker that left leaves a nil. + private var snapshot: [MarkerEntity?] = [] + /// Sorted `(mortonKey << indexBits) | slot`. private var packed: [Int64] = [] + /// Entries for markers that arrived since the last merge, unsorted. + private var pending: [Int64] = [] + /// Slots emptied by markers that left. + private var emptySlots = 0 private var dirty = true init(source: @escaping () -> [MarkerEntity]) { @@ -58,13 +92,84 @@ final class MarkerGridIndex { var isBuilt: Bool { !dirty } /// Roughly what the index costs: two words a marker, key and reference. - func estimatedBytes() -> Int64 { Int64(packed.count) * 8 + Int64(snapshot.count) * 8 } + func estimatedBytes() -> Int64 { + Int64(packed.count) * 8 + Int64(snapshot.count) * 8 + Int64(pending.count) * 8 + } /// Marks the index stale. The rebuild happens on the next query. func invalidate() { dirty = true } + /// Files a marker that was not in the index. + func add(_ entity: MarkerEntity) { + // Until the first build, or while a rebuild is due, the rebuild reads the + // manager's collection and will see this marker there. + if dirty { return } + if snapshot.count == Self.maxSlots { + dirty = true + return + } + let slot = snapshot.count + snapshot.append(entity) + pending.append((MarkerGrid.mortonKey(for: entity.state.position) << Self.indexBits) | Int64(slot)) + } + + /// Forgets a marker. + func remove(_ entity: MarkerEntity) { + if dirty { return } + guard let slot = locate(entity) else { + // Not where it should be: the index lost track of it somewhere, so + // stop trusting it rather than return a marker that has gone. + dirty = true + return + } + snapshot[slot] = nil + emptySlots += 1 + } + + /// `previous` is replaced by `entity` under the same id — the same object + /// after a change, or a new one. + /// + /// Most updates leave the marker in its cell: an icon or a flag changed. + /// Then only the reference in its slot is swapped, and nothing waits for a + /// merge. + func replace(_ previous: MarkerEntity, with entity: MarkerEntity) { + if dirty { return } + if let slot = locate(previous, inCell: MarkerGrid.mortonKey(for: entity.state.position)) { + snapshot[slot] = entity + return + } + remove(previous) + add(entity) + } + + /// The slot holding `entity`, if any. + private func locate(_ entity: MarkerEntity) -> Int? { + if let slot = locate(entity, inCell: MarkerGrid.mortonKey(for: entity.state.position)) { + return slot + } + // Its position changed in place since it was filed, so its entry is in + // some other cell's run. Scan the slots. + return snapshot.firstIndex { $0 === entity } + } + + /// The slot holding `entity` if it was filed under bottom-level `key`. + private func locate(_ entity: MarkerEntity, inCell key: Int64) -> Int? { + for entry in pending where entry >> Self.indexBits == key { + let slot = Int(entry & Self.indexMask) + if snapshot[slot] === entity { return slot } + } + let limit = (key + 1) << Self.indexBits + var at = lowerBound(key << Self.indexBits) + while at < packed.count, packed[at] < limit { + let slot = Int(packed[at] & Self.indexMask) + if snapshot[slot] === entity { return slot } + at += 1 + } + return nil + } + /// Markers whose position falls inside `bounds`. func inBounds(_ bounds: GeoRectBounds) -> [MarkerEntity] { guard let southWest = bounds.southWest, let northEast = bounds.northEast else { return [] } @@ -171,12 +276,20 @@ final class MarkerGridIndex { if at >= packed.count { continue } let limit = (key + 1) << shift if packed[at] >= limit { continue } - // The run's last entry. No containment test anywhere: that is - // the whole point, and it is also why this is cheaper than the - // version that scanned every border cell. + // The run's last live entry. No containment test anywhere: that + // is the whole point, and it is also why this is cheaper than + // the version that scanned every border cell. A marker that + // left empties its slot without moving the entries, so step + // back over those. var last = at while last + 1 < packed.count && packed[last + 1] < limit { last += 1 } - found.append(snapshot[Int(packed[last] & Self.indexMask)]) + while last >= at { + if let entity = snapshot[Int(packed[last] & Self.indexMask)] { + found.append(entity) + break + } + last -= 1 + } } } return found @@ -272,13 +385,21 @@ final class MarkerGridIndex { var at = lowerBound(key << shift) let limit = (key + 1) << shift while at < packed.count, packed[at] < limit { - body(snapshot[Int(packed[at] & Self.indexMask)]) + if let entity = snapshot[Int(packed[at] & Self.indexMask)] { body(entity) } at += 1 } } private func rebuildIfNeeded() { - guard dirty else { return } + if !dirty, emptySlots > Self.rebuildMinSlots, emptySlots * 2 > snapshot.count { dirty = true } + if dirty { + rebuild() + } else if !pending.isEmpty { + merge() + } + } + + private func rebuild() { let entities = source() var keys = [Int64]() keys.reserveCapacity(entities.count) @@ -288,9 +409,36 @@ final class MarkerGridIndex { keys.sort() snapshot = entities packed = keys + pending = [] + emptySlots = 0 dirty = false } + /// Folds the markers that arrived into `packed`: sort those alone, then one + /// pass over both sorted runs. Entries whose slot was emptied are dropped on + /// the way, from either side. + private func merge() { + pending.sort() + var merged: [Int64] = [] + merged.reserveCapacity(packed.count + pending.count) + var i = 0 + var j = 0 + while i < packed.count || j < pending.count { + let takeOld = j >= pending.count || (i < packed.count && packed[i] < pending[j]) + let entry: Int64 + if takeOld { + entry = packed[i] + i += 1 + } else { + entry = pending[j] + j += 1 + } + if snapshot[Int(entry & Self.indexMask)] != nil { merged.append(entry) } + } + packed = merged + pending.removeAll(keepingCapacity: true) + } + private func lowerBound(_ target: Int64) -> Int { var low = 0 var high = packed.count diff --git a/Sources/MapConductorCore/marker/MarkerManager.swift b/Sources/MapConductorCore/marker/MarkerManager.swift index f43c014..977903d 100644 --- a/Sources/MapConductorCore/marker/MarkerManager.swift +++ b/Sources/MapConductorCore/marker/MarkerManager.swift @@ -69,7 +69,7 @@ public final class MarkerManager { guard usableLocked("removeEntity") else { return nil } let removed = entities.removeValue(forKey: id) if let removed { - gridIndex.invalidate() + gridIndex.remove(removed) } return removed } @@ -78,16 +78,24 @@ public final class MarkerManager { lock.lock() defer { lock.unlock() } guard usableLocked("registerEntity") else { return } - entities[entity.state.id] = entity - gridIndex.invalidate() + fileLocked(entities.updateValue(entity, forKey: entity.state.id), entity) } public func updateEntity(_ entity: MarkerEntity) { lock.lock() defer { lock.unlock() } guard usableLocked("updateEntity") else { return } - entities[entity.state.id] = entity - gridIndex.invalidate() + // A marker that moved changes which cell it belongs to. + fileLocked(entities.updateValue(entity, forKey: entity.state.id), entity) + } + + /// Tells the index that `entity` took the place of `previous`, if there was one. + private func fileLocked(_ previous: MarkerEntity?, _ entity: MarkerEntity) { + if let previous { + gridIndex.replace(previous, with: entity) + } else { + gridIndex.add(entity) + } } public func metersPerPixel( diff --git a/Tests/MapConductorCoreTests/MarkerGridIndexTests.swift b/Tests/MapConductorCoreTests/MarkerGridIndexTests.swift index d346361..ba8c6e7 100644 --- a/Tests/MapConductorCoreTests/MarkerGridIndexTests.swift +++ b/Tests/MapConductorCoreTests/MarkerGridIndexTests.swift @@ -286,4 +286,169 @@ final class MarkerGridIndexTests: XCTestCase { } XCTAssertEqual(nonEmpty, 7, "空の集合どうしを比べているだけになっている") } + + /// Changes are applied to the built index, not rebuilt from the collection. + /// + /// The index is queried first so every change lands on a built index, and + /// the collection counts how often it is read: a rebuild reads it, applying + /// a change does not. After every kind of change the answers still have to + /// be a scan's — a slot left pointing at a marker that went, or an entry + /// left in the cell a marker moved out of, does not fail loudly; it returns + /// a marker that is not there. + /// + /// Pairs with android-sdk's `changesAreAppliedWithoutARebuild`. + func testChangesAreAppliedWithoutARebuild() { + var markers = scatter(count: 5000, latitude: 35.68, longitude: 139.76, spread: 0.6) + var reads = 0 + let index = MarkerGridIndex { + reads += 1 + return markers + } + let boxes = [ + bounds(35.6812, 139.7612, 35.6814, 139.7614), + bounds(35.68, 139.76, 35.69, 139.77), + bounds(35.6, 139.6, 35.8, 139.9), + bounds(35.0, 139.0, 36.5, 140.5), + ] + let probes = [ + GeoPoint(latitude: 35.68, longitude: 139.76), + GeoPoint(latitude: 35.5, longitude: 139.6), + GeoPoint(latitude: 35.9, longitude: 140.0), + ] + + func cellOf(_ entity: MarkerEntity) -> String { + MarkerGrid.geocell( + latitude: entity.state.position.latitude, + longitude: entity.state.position.longitude, + characters: 9 + ) + } + + func assertMatchesScan(_ label: String) { + // By identity: a replaced marker has the same id as the one it replaced. + let live = Set(markers.map(ObjectIdentifier.init)) + for (at, box) in boxes.enumerated() { + let expected = ids(markers.filter { box.contains(point: $0.state.position) }) + let found = index.inBounds(box) + XCTAssertEqual(ids(found), expected, "\(label), box \(at)") + XCTAssertEqual(found.count, expected.count, "\(label), box \(at): a marker twice") + XCTAssertTrue(found.allSatisfy { live.contains(ObjectIdentifier($0)) }, "\(label), box \(at): a replaced object") + } + for point in probes + markers.prefix(20).map(\.state.position) { + let best = markers.map { squared($0, point) }.min()! + XCTAssertEqual(index.nearest(position: point).map { squared($0, point) }, best, "\(label), nearest") + } + let box = bounds(35.60, 139.68, 35.76, 139.84) + guard let kept = index.inBoundsThinned(box, minSeparationDegrees: 0.005) else { + return XCTFail("\(label): thinning declined") + } + XCTAssertTrue(kept.allSatisfy { live.contains(ObjectIdentifier($0)) }, "\(label): thinning returned a marker that left") + let inside = Set(markers.filter { box.contains(point: $0.state.position) }.map(cellOf)) + XCTAssertTrue(Set(kept.map(cellOf)).isSuperset(of: inside), "\(label): a populated cell went unrepresented") + } + + assertMatchesScan("built") + XCTAssertEqual(reads, 1) + + var seed: UInt64 = 99 + func next() -> Double { + seed = seed &* 6_364_136_223_846_793_005 &+ 1_442_695_040_888_963_407 + return Double(seed >> 11) / Double(UInt64(1) << 53) + } + func anywhere() -> GeoPoint { + GeoPoint(latitude: 35.38 + next() * 0.6, longitude: 139.46 + next() * 0.6) + } + var nextId = 10_000 + for round in 0..<5 { + for _ in 0..<200 { + switch Int(next() * 5) { + case 0: + // A new marker. + let position = anywhere() + let added = entity(nextId, position.latitude, position.longitude) + nextId += 1 + markers.append(added) + index.add(added) + case 1: + // A marker leaves. + let gone = markers.remove(at: Int(next() * Double(markers.count))) + index.remove(gone) + case 2: + // Replaced by a new object in the same place: an icon change. + let at = Int(next() * Double(markers.count)) + let old = markers[at] + let replacement = entity(Int(old.state.id)!, old.state.position.latitude, old.state.position.longitude) + markers[at] = replacement + index.replace(old, with: replacement) + case 3: + // Replaced by a new object somewhere else. + let at = Int(next() * Double(markers.count)) + let old = markers[at] + let position = anywhere() + let moved = entity(Int(old.state.id)!, position.latitude, position.longitude) + markers[at] = moved + index.replace(old, with: moved) + default: + // Moved in place, as a drag does, then reported. + let dragged = markers[Int(next() * Double(markers.count))] + dragged.state.position = anywhere() + index.replace(dragged, with: dragged) + } + } + assertMatchesScan("round \(round)") + } + XCTAssertEqual(reads, 1, "a change was applied by rebuilding") + } + + /// A cell whose representative left is still represented. + /// + /// The thinned query takes the last entry of a cell's run. A marker that + /// leaves empties its slot but keeps its entry until the next merge, and + /// with nothing added since, there is no merge: the query has to step back + /// past it to the one before, or the cell drops out of the map. + func testThinnedQuerySkipsAMarkerThatLeft() { + let first = entity(0, 35.68, 139.76) + let second = entity(1, 35.68, 139.76) + var markers = [first, second] + let index = MarkerGridIndex { markers } + let box = bounds(35.67, 139.75, 35.69, 139.77) + XCTAssertEqual(index.inBoundsThinned(box, minSeparationDegrees: 0.01)?.map(ObjectIdentifier.init), [ObjectIdentifier(second)]) + + markers.removeLast() + index.remove(second) + XCTAssertEqual(index.inBoundsThinned(box, minSeparationDegrees: 0.01)?.map(ObjectIdentifier.init), [ObjectIdentifier(first)]) + } + + /// A marker the index cannot find makes it rebuild rather than trust itself. + func testRemovingAnUnknownMarkerRebuilds() { + let markers = scatter(count: 1000, latitude: 35.68, longitude: 139.76, spread: 0.6) + var reads = 0 + let index = MarkerGridIndex { + reads += 1 + return markers + } + let box = bounds(35.0, 139.0, 36.5, 140.5) + XCTAssertEqual(index.inBounds(box).count, markers.count) + + index.remove(entity(99_999, 35.68, 139.76)) + XCTAssertFalse(index.isBuilt) + XCTAssertEqual(index.inBounds(box).count, markers.count) + XCTAssertEqual(reads, 2) + } + + /// Once half the slots are empty, a rebuild compacts them. + func testManyRemovalsCompactByRebuilding() { + var markers = scatter(count: 20_000, latitude: 35.68, longitude: 139.76, spread: 0.6) + var reads = 0 + let index = MarkerGridIndex { + reads += 1 + return markers + } + let box = bounds(35.0, 139.0, 36.5, 140.5) + _ = index.inBounds(box) + + for _ in 0..<12_000 { index.remove(markers.removeLast()) } + XCTAssertEqual(ids(index.inBounds(box)), ids(markers)) + XCTAssertEqual(reads, 2) + } }