From 9336a2dc1e5227f3b864862573255fd58dcf84b3 Mon Sep 17 00:00:00 2001 From: Masashi Katsumata Date: Wed, 4 Nov 2026 18:56:00 -0800 Subject: [PATCH] Apply marker changes to the grid index instead of rebuilding it MarkerGridIndex was thrown away on every add, remove or update, so the first query after any change paid the full build. An app that swaps markers on every pan paid it on its next tap and on every tile, and a drag paid it on every frame. On Android's Pixel 5a, after 300 swaps, that was 212 ms at 144,000 markers. A marker that arrives now takes the next slot, and its entry waits in a pending list; the next query sorts only those and merges them into the sorted array in one pass. A marker that leaves empties its slot, which queries skip and the next merge drops. A replacement that stays in its cell swaps the reference in its slot and nothing more. The index rebuilds only when half its slots are empty or when it cannot find a marker it was told to forget. Nothing is kept per marker to find its entry: it is in the run of the cell its position falls in, matched by identity. A marker moved in place before it is reported, as a drag does, has its entry in another cell's run; for that one the slots are scanned. The thinned query took a cell's last entry; with emptied slots it steps back to the last live one, so a cell whose representative left keeps one. Same change as MapConductor/android-sdk-core#20. --- .../marker/MarkerGridIndex.swift | 166 +++++++++++++++++- .../marker/MarkerManager.swift | 18 +- .../MarkerGridIndexTests.swift | 165 +++++++++++++++++ 3 files changed, 335 insertions(+), 14 deletions(-) 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) + } }