diff --git a/src/main/java/com/mapconductor/core/marker/MarkerGridIndex.kt b/src/main/java/com/mapconductor/core/marker/MarkerGridIndex.kt index 13cffb3..d117743 100644 --- a/src/main/java/com/mapconductor/core/marker/MarkerGridIndex.kt +++ b/src/main/java/com/mapconductor/core/marker/MarkerGridIndex.kt @@ -33,16 +33,49 @@ import java.util.Arrays * snapshot when it rebuilds, so nothing here duplicates the map the manager * already keeps — an id-to-slot map of its own cost several megabytes at this * size, for information that already existed. + * + * ## 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 a Pixel 5a, 83 ms at + * 60,000 markers and 182 ms at 144,000, up to 780 ms, after 300 swaps. + * + * 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 either. 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. */ @InternalMapConductorApi internal class MarkerGridIndex( private val source: () -> Collection>, ) { + /** Slot -> marker. A marker that left leaves a null. */ + @Volatile private var snapshot: Array?> = emptyArray() - /** Sorted `(mortonKey shl INDEX_BITS) or position-in-snapshot`. */ + /** Sorted `(mortonKey shl INDEX_BITS) or slot`. */ + @Volatile private var packed = LongArray(0) + /** Entries for markers that arrived since the last merge, unsorted. */ + private var pending = LongArray(0) + private var pendingCount = 0 + + /** Slots handed out, live or empty. */ + private var slotCount = 0 + + /** Slots emptied by markers that left. */ + private var emptySlots = 0 + @Volatile private var dirty = true @@ -51,13 +84,104 @@ internal class MarkerGridIndex( get() = !dirty /** Roughly what the index costs: two longs a marker, key and reference. */ - fun estimatedBytes(): Long = packed.size * 8L + snapshot.size * 8L + fun estimatedBytes(): Long = packed.size * 8L + snapshot.size * 8L + pending.size * 8L /** Marks the index stale. The rebuild happens on the next query. */ + @Synchronized fun invalidate() { dirty = true } + /** Files a marker that was not in the index. */ + @Synchronized + fun add(entity: MarkerEntityInterface) { + // 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 (slotCount == MAX_SLOTS) { + dirty = true + return + } + if (slotCount == snapshot.size) snapshot = snapshot.copyOf(maxOf(16, slotCount * 2)) + val slot = slotCount++ + snapshot[slot] = entity + if (pendingCount == pending.size) pending = pending.copyOf(maxOf(16, pendingCount * 2)) + pending[pendingCount++] = (MarkerGrid.mortonKeyFor(entity.state.position) shl INDEX_BITS) or slot.toLong() + } + + /** Forgets a marker. */ + @Synchronized + fun remove(entity: MarkerEntityInterface) { + if (dirty) return + val slot = locate(entity) + if (slot < 0) { + // 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] = null + emptySlots++ + } + + /** + * [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. + */ + @Synchronized + fun replace( + previous: MarkerEntityInterface, + entity: MarkerEntityInterface, + ) { + if (dirty) return + val slot = locateInCell(previous, MarkerGrid.mortonKeyFor(entity.state.position)) + if (slot >= 0) { + snapshot[slot] = entity + return + } + remove(previous) + add(entity) + } + + /** The slot holding [entity], or -1. */ + private fun locate(entity: MarkerEntityInterface): Int { + val slot = locateInCell(entity, MarkerGrid.mortonKeyFor(entity.state.position)) + if (slot >= 0) return slot + // Its position changed in place since it was filed, so its entry is in + // some other cell's run. Scan the slots. + val slots = snapshot + for (at in 0 until slotCount) { + if (slots[at] === entity) return at + } + return -1 + } + + /** The slot holding [entity] if it was filed under bottom-level [key], or -1. */ + private fun locateInCell( + entity: MarkerEntityInterface, + key: Long, + ): Int { + val slots = snapshot + for (at in 0 until pendingCount) { + val entry = pending[at] + if (entry ushr INDEX_BITS != key) continue + val slot = (entry and INDEX_MASK).toInt() + if (slots[slot] === entity) return slot + } + val entries = packed + val limit = (key + 1) shl INDEX_BITS + var at = lowerBound(entries, key shl INDEX_BITS) + while (at < entries.size && entries[at] < limit) { + val slot = (entries[at] and INDEX_MASK).toInt() + if (slots[slot] === entity) return slot + at++ + } + return -1 + } + /** Markers whose position falls inside [bounds]. */ fun inBounds(bounds: GeoRectBounds): List> { val southWest = bounds.southWest ?: return emptyList() @@ -144,21 +268,32 @@ internal class MarkerGridIndex( if ((latTo - latFrom + 1) * columnCount > MAX_CELLS_PER_THINNED_QUERY) return null rebuildIfNeeded() + val entries = packed + val slots = snapshot val shift = INDEX_BITS + 2 * (MarkerGrid.GRID_DEPTH - level) val found = ArrayList>() for (latCell in latFrom..latTo) { for (step in 0 until columnCount) { val key = MarkerGrid.morton(latCell, MarkerGrid.wrap(firstColumn + step, level), level) - val at = lowerBound(key shl shift) - if (at >= packed.size) continue + val at = lowerBound(entries, key shl shift) + if (at >= entries.size) continue val limit = (key + 1) shl 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. + if (entries[at] >= limit) continue + // 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.size && packed[last + 1] < limit) last++ - snapshot[(packed[last] and INDEX_MASK).toInt()]?.let { found.add(it) } + while (last + 1 < entries.size && entries[last + 1] < limit) last++ + while (last >= at) { + val entity = slots[(entries[last] and INDEX_MASK).toInt()] + if (entity != null) { + found.add(entity) + break + } + last-- + } } } return found @@ -260,18 +395,28 @@ internal class MarkerGridIndex( level: Int, body: (MarkerEntityInterface) -> Unit, ) { + val entries = packed + val slots = snapshot val shift = INDEX_BITS + 2 * (MarkerGrid.GRID_DEPTH - level) - var at = lowerBound(key shl shift) + var at = lowerBound(entries, key shl shift) val limit = (key + 1) shl shift - while (at < packed.size && packed[at] < limit) { - snapshot[(packed[at] and INDEX_MASK).toInt()]?.let(body) + while (at < entries.size && entries[at] < limit) { + slots[(entries[at] and INDEX_MASK).toInt()]?.let(body) at++ } } @Synchronized private fun rebuildIfNeeded() { - if (!dirty) return + if (!dirty && emptySlots > REBUILD_MIN_SLOTS && emptySlots * 2 > slotCount) dirty = true + if (dirty) { + rebuild() + return + } + if (pendingCount > 0) merge() + } + + private fun rebuild() { val entities = source() val taken = arrayOfNulls>(entities.size) val keys = LongArray(entities.size) @@ -286,15 +431,44 @@ internal class MarkerGridIndex( Arrays.sort(keys, 0, at) snapshot = taken packed = if (at == keys.size) keys else keys.copyOf(at) + slotCount = at + emptySlots = 0 + pending = LongArray(0) + pendingCount = 0 dirty = false } - private fun lowerBound(target: Long): Int { + /** + * 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 fun merge() { + Arrays.sort(pending, 0, pendingCount) + val slots = snapshot + val old = packed + val merged = LongArray(old.size + pendingCount) + var size = 0 + var i = 0 + var j = 0 + while (i < old.size || j < pendingCount) { + val takeOld = j >= pendingCount || (i < old.size && old[i] < pending[j]) + val entry = if (takeOld) old[i++] else pending[j++] + if (slots[(entry and INDEX_MASK).toInt()] != null) merged[size++] = entry + } + packed = if (size == merged.size) merged else merged.copyOf(size) + pendingCount = 0 + } + + private fun lowerBound( + entries: LongArray, + target: Long, + ): Int { var low = 0 - var high = packed.size + var high = entries.size while (low < high) { val mid = (low + high) ushr 1 - if (packed[mid] < target) low = mid + 1 else high = mid + if (entries[mid] < target) low = mid + 1 else high = mid } return low } @@ -327,5 +501,15 @@ internal class MarkerGridIndex( // 24 bits of position, enough for 16.7M markers, under the cell key. private const val INDEX_BITS = 24 private const val INDEX_MASK = (1L shl INDEX_BITS) - 1 + + /** Slots the index can hand out before it has to rebuild to renumber them. */ + private const val MAX_SLOTS = 1 shl INDEX_BITS + + /** + * 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 const val REBUILD_MIN_SLOTS = 4096 } } diff --git a/src/main/java/com/mapconductor/core/marker/MarkerManager.kt b/src/main/java/com/mapconductor/core/marker/MarkerManager.kt index 93777c5..5075864 100644 --- a/src/main/java/com/mapconductor/core/marker/MarkerManager.kt +++ b/src/main/java/com/mapconductor/core/marker/MarkerManager.kt @@ -66,7 +66,7 @@ open class MarkerManager( if (!usable("removeEntity")) return null val removed = entities.remove(id) if (removed != null) { - gridIndex.invalidate() + gridIndex.remove(removed) } return removed } @@ -108,20 +108,26 @@ open class MarkerManager( open fun registerEntity(entity: MarkerEntityInterface) { if (!usable("registerEntity")) return semaphore.write { - entities[entity.state.id] = entity - gridIndex.invalidate() + file(entities.put(entity.state.id, entity), entity) } } open fun updateEntity(entity: MarkerEntityInterface) { if (!usable("updateEntity")) return semaphore.write { - entities[entity.state.id] = entity // A marker that moved changes which cell it belongs to. - gridIndex.invalidate() + file(entities.put(entity.state.id, entity), entity) } } + /** Tells the index that [entity] took the place of [previous], if there was one. */ + private fun file( + previous: MarkerEntityInterface?, + entity: MarkerEntityInterface, + ) { + if (previous == null) gridIndex.add(entity) else gridIndex.replace(previous, entity) + } + open fun allEntities(): List> { if (!usable("allEntities")) return emptyList() semaphore.read { diff --git a/src/test/java/com/mapconductor/core/marker/MarkerGridIndexTest.kt b/src/test/java/com/mapconductor/core/marker/MarkerGridIndexTest.kt index a87fcf4..14499b5 100644 --- a/src/test/java/com/mapconductor/core/marker/MarkerGridIndexTest.kt +++ b/src/test/java/com/mapconductor/core/marker/MarkerGridIndexTest.kt @@ -250,6 +250,180 @@ class MarkerGridIndexTest { assertEquals(2, index.inBounds(box).size) } + /** + * 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. + */ + @Test + fun changesAreAppliedWithoutARebuild() { + val markers = scatter(5000, 35.68, 139.76, 0.6).toMutableList() + var reads = 0 + val index = + MarkerGridIndex { + reads++ + markers + } + val boxes = + listOf( + 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), + ) + val probes = + listOf( + GeoPoint.fromLatLong(35.68, 139.76), + GeoPoint.fromLatLong(35.5, 139.6), + GeoPoint.fromLatLong(35.9, 140.0), + ) + + fun assertMatchesScan(label: String) { + // By identity: a replaced marker has the same id as the one it replaced. + val live = markers.toSet() + for ((at, box) in boxes.withIndex()) { + val expected = ids(markers.filter { box.contains(it.state.position) }) + val found = index.inBounds(box) + assertEquals("$label, box $at", expected, ids(found)) + assertEquals("$label, box $at: a marker twice", expected.size, found.size) + assertTrue("$label, box $at: a replaced object", found.all { it in live }) + } + for (point in probes + markers.take(20).map { it.state.position as GeoPoint }) { + val best = markers.minOf { squared(it, point) } + assertEquals("$label, nearest", best, squared(index.nearest(point)!!, point), 0.0) + } + val box = bounds(35.60, 139.68, 35.76, 139.84) + val kept = index.inBoundsThinned(box, 0.005)!! + assertTrue("$label: thinning returned a marker that left", kept.all { it in live }) + + fun cellOf(entity: MarkerEntityInterface): String = + MarkerGrid.geocell(entity.state.position.latitude, entity.state.position.longitude, characters = 9) + val inside = markers.filter { box.contains(it.state.position) }.map(::cellOf).toSet() + assertTrue("$label: a populated cell went unrepresented", kept.map(::cellOf).toSet().containsAll(inside)) + } + + assertMatchesScan("built") + assertEquals(1, reads) + + var seed = 99L + + fun next(): Double { + seed = seed * 6364136223846793005L + 1442695040888963407L + return (seed ushr 11).toDouble() / (1L shl 53).toDouble() + } + + fun anywhere() = GeoPoint.fromLatLong(35.38 + next() * 0.6, 139.46 + next() * 0.6) + var nextId = 10_000 + repeat(5) { round -> + repeat(200) { + when ((next() * 5).toInt()) { + // A new marker. + 0 -> { + val position = anywhere() + val added = entity(nextId++, position.latitude, position.longitude) + markers.add(added) + index.add(added) + } + // A marker leaves. + 1 -> { + val gone = markers.removeAt((next() * markers.size).toInt()) + index.remove(gone) + } + // Replaced by a new object in the same place: an icon change. + 2 -> { + val at = (next() * markers.size).toInt() + val old = markers[at] + val replacement = + entity(old.state.id.toInt(), old.state.position.latitude, old.state.position.longitude) + markers[at] = replacement + index.replace(old, replacement) + } + // Replaced by a new object somewhere else. + 3 -> { + val at = (next() * markers.size).toInt() + val old = markers[at] + val position = anywhere() + val moved = entity(old.state.id.toInt(), position.latitude, position.longitude) + markers[at] = moved + index.replace(old, moved) + } + // Moved in place, as a drag does, then reported. + else -> { + val dragged = markers[(next() * markers.size).toInt()] + dragged.state.position = anywhere() + index.replace(dragged, dragged) + } + } + } + assertMatchesScan("round $round") + } + assertEquals("a change was applied by rebuilding", 1, reads) + } + + /** + * 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. + */ + @Test + fun thinnedQuerySkipsAMarkerThatLeft() { + val first = entity(0, 35.68, 139.76) + val second = entity(1, 35.68, 139.76) + val markers = mutableListOf(first, second) + val index = MarkerGridIndex { markers } + val box = bounds(35.67, 139.75, 35.69, 139.77) + assertEquals(listOf(second), index.inBoundsThinned(box, 0.01)) + + markers.remove(second) + index.remove(second) + assertEquals(listOf(first), index.inBoundsThinned(box, 0.01)) + } + + /** A marker the index cannot find makes it rebuild rather than trust itself. */ + @Test + fun removingAnUnknownMarkerRebuilds() { + val markers = scatter(1000, 35.68, 139.76, 0.6).toMutableList() + var reads = 0 + val index = + MarkerGridIndex { + reads++ + markers + } + val box = bounds(35.0, 139.0, 36.5, 140.5) + assertEquals(markers.size, index.inBounds(box).size) + + index.remove(entity(99_999, 35.68, 139.76)) + assertFalse(index.isBuilt) + assertEquals(markers.size, index.inBounds(box).size) + assertEquals(2, reads) + } + + /** Once half the slots are empty, a rebuild compacts them. */ + @Test + fun manyRemovalsCompactByRebuilding() { + val markers = scatter(20_000, 35.68, 139.76, 0.6).toMutableList() + var reads = 0 + val index = + MarkerGridIndex { + reads++ + markers + } + val box = bounds(35.0, 139.0, 36.5, 140.5) + index.inBounds(box) + + repeat(12_000) { index.remove(markers.removeAt(markers.size - 1)) } + assertEquals(ids(markers), ids(index.inBounds(box))) + assertEquals(2, reads) + } + private fun squared( entity: MarkerEntityInterface, point: GeoPoint,