diff --git a/api/android-sdk-core.api b/api/android-sdk-core.api index e1ffa9d..bbdf101 100644 --- a/api/android-sdk-core.api +++ b/api/android-sdk-core.api @@ -1095,6 +1095,7 @@ public abstract class com.mapconductor.core.map.MapViewState { + public static final com.mapconductor.core.tileserver.ScratchPool$Companion Companion; + public com.mapconductor.core.tileserver.ScratchPool(int); + public com.mapconductor.core.tileserver.ScratchPool(int, int, kotlin.jvm.internal.DefaultConstructorMarker); + public final T acquire(kotlin.jvm.functions.Function1); + public final boolean release(T); + public final int getSize(); + public com.mapconductor.core.tileserver.ScratchPool(); +} +public final class com.mapconductor.core.tileserver.ScratchPool$Companion { + public final int getDEFAULT_CAPACITY(); +} public final class com.mapconductor.core.tileserver.TileCoordinate { public com.mapconductor.core.tileserver.TileCoordinate(int, int); public final int getY(); diff --git a/src/main/java/com/mapconductor/core/circle/CircleController.kt b/src/main/java/com/mapconductor/core/circle/CircleController.kt index 2075365..87ec272 100644 --- a/src/main/java/com/mapconductor/core/circle/CircleController.kt +++ b/src/main/java/com/mapconductor/core/circle/CircleController.kt @@ -35,7 +35,8 @@ abstract class CircleController( data.forEach { state -> if (previous.contains(state.id)) { - val prevEntity = circleManager.getEntity(state.id)!! + // Gone only if destroy() cleared the manager meanwhile; nothing left to update. + val prevEntity = circleManager.getEntity(state.id) ?: return@withPermit if (state.fingerPrint() == prevEntity.fingerPrint) { // 描画結果が不変なら renderer を呼ばず最新の state だけ採用する // （react-sdk と同じ。composition ごとに全オーバーレイへ onChange を @@ -158,7 +159,9 @@ abstract class CircleController( override fun find(position: GeoPointInterface): CircleEntityInterface? = circleManager.find(position) override fun destroy() { - // No native resources to clean up for circles + // A destroyed controller can still be referenced (by its map controller, + // or a state that has not let go yet); without this it keeps every entity. + circleManager.clear() } override fun has(id: String): Boolean = circleManager.hasEntity(id) diff --git a/src/main/java/com/mapconductor/core/groundimage/GroundImageController.kt b/src/main/java/com/mapconductor/core/groundimage/GroundImageController.kt index b51818a..1c5cbc9 100644 --- a/src/main/java/com/mapconductor/core/groundimage/GroundImageController.kt +++ b/src/main/java/com/mapconductor/core/groundimage/GroundImageController.kt @@ -36,7 +36,8 @@ abstract class GroundImageController( data.forEach { state -> if (previous.contains(state.id)) { - val prevEntity = groundImageManager.getEntity(state.id)!! + // Gone only if destroy() cleared the manager meanwhile; nothing left to update. + val prevEntity = groundImageManager.getEntity(state.id) ?: return@withPermit if (state.fingerPrint() == prevEntity.fingerPrint) { // 描画結果が不変なら renderer を呼ばず最新の state だけ採用する（react-sdk と同じ）。 groundImageManager @@ -157,7 +158,9 @@ abstract class GroundImageController( .find(position) override fun destroy() { - // No native resources to clean up + // A destroyed controller can still be referenced (by its map controller, + // or a state that has not let go yet); without this it keeps every entity. + groundImageManager.clear() } override fun has(id: String): Boolean = groundImageManager.hasEntity(id) diff --git a/src/main/java/com/mapconductor/core/map/MapViewState.kt b/src/main/java/com/mapconductor/core/map/MapViewState.kt index bd6e7e7..9ce07ed 100644 --- a/src/main/java/com/mapconductor/core/map/MapViewState.kt +++ b/src/main/java/com/mapconductor/core/map/MapViewState.kt @@ -148,6 +148,23 @@ abstract class MapViewState( this.attachedMapController = null } + /** + * 破棄した [controller] を手放す。地図が壊されたあとで呼ぶ。 + * + * state は地図より長生きする（アプリが保持し、プロバイダを切り替えても残る）。 + * ここで手放さないと、state が最後のコントローラ（とその全オーバーレイ）を + * 抱えたままになる。Compose では `MapViewBase` が地図をコンポジションから + * 外したときに呼ぶので、プロバイダが呼ぶ必要はない。 + * + * その間に別のコントローラが接続されていれば何もしない（新しい地図を + * 古い地図の後始末で切り離さないため）。 + */ + fun detachController(controller: MapViewControllerInterface) { + if (this.attachedMapController === controller) { + this.attachedMapController = null + } + } + /** * 地図から通知された現在のカメラを保持する（地図を動かさない）。 * diff --git a/src/main/java/com/mapconductor/core/marker/MarkerTileRenderer.kt b/src/main/java/com/mapconductor/core/marker/MarkerTileRenderer.kt index 0357e52..f664be5 100644 --- a/src/main/java/com/mapconductor/core/marker/MarkerTileRenderer.kt +++ b/src/main/java/com/mapconductor/core/marker/MarkerTileRenderer.kt @@ -6,6 +6,7 @@ import androidx.core.graphics.createBitmap import com.mapconductor.core.ResourceProvider import com.mapconductor.core.features.GeoPoint import com.mapconductor.core.features.GeoRectBounds +import com.mapconductor.core.tileserver.ScratchPool import com.mapconductor.core.tileserver.TilePngEncoder import com.mapconductor.core.tileserver.TileProviderInterface import com.mapconductor.core.tileserver.TileRequest @@ -686,10 +687,10 @@ class MarkerTileRenderer( // ByteArrayOutputStream starts at 32 bytes and doubles its backing array on every // compress() call until it reaches the tile's steady-state size (often hundreds of KB // for dense tiles), so a new instance per call re-triggers that regrowth every time. - // Keeping one per thread (reset, not recreated, before each use) lets the backing - // array settle at its steady-state size and be reused across tiles. - private val tileByteStream = - ThreadLocal.withInitial { ByteArrayOutputStream(16 * 1024) } + // Keeping a few (reset, not recreated, before each use) lets the backing array + // settle at its steady-state size and be reused across tiles. A pool, not one per + // thread: map SDK threads that outlive their map would keep theirs (ScratchPool). + private val tileByteStreams = ScratchPool() private fun bitmapToByteArray(bitmap: Bitmap): ByteArray { // Rust first: it is 4-6x faster than Bitmap.compress on the tiles this @@ -698,15 +699,17 @@ class MarkerTileRenderer( // declined the bitmap, and the platform encoder takes over. TilePngEncoder.encode(bitmap)?.let { return it } - // ThreadLocal.get() is a Java generic method, so Kotlin sees its return type as - // nullable even though withInitial() guarantees a value; !! is safe here. - val outputStream = tileByteStream.get()!! - outputStream.reset() - bitmap.compress(Bitmap.CompressFormat.PNG, 100, outputStream) - // toByteArray() still copies out a right-sized array: callers cache this array or - // hand it to a socket write, both of which need a stable array that isn't mutated - // by the next renderTile() call on this thread. - return outputStream.toByteArray() + val outputStream = tileByteStreams.acquire() ?: ByteArrayOutputStream(16 * 1024) + try { + outputStream.reset() + bitmap.compress(Bitmap.CompressFormat.PNG, 100, outputStream) + // toByteArray() still copies out a right-sized array: callers cache this array or + // hand it to a socket write, both of which need a stable array that isn't mutated + // by the next renderTile() call. + return outputStream.toByteArray() + } finally { + tileByteStreams.release(outputStream) + } } private fun acquireBitmap(size: Int): Bitmap { diff --git a/src/main/java/com/mapconductor/core/polygon/PolygonController.kt b/src/main/java/com/mapconductor/core/polygon/PolygonController.kt index 96eb44f..c9ad5c9 100644 --- a/src/main/java/com/mapconductor/core/polygon/PolygonController.kt +++ b/src/main/java/com/mapconductor/core/polygon/PolygonController.kt @@ -35,7 +35,8 @@ abstract class PolygonController( data.forEach { state -> if (previous.contains(state.id)) { - val prevEntity = polygonManager.getEntity(state.id)!! + // Gone only if destroy() cleared the manager meanwhile; nothing left to update. + val prevEntity = polygonManager.getEntity(state.id) ?: return@withPermit if (state.fingerPrint() == prevEntity.fingerPrint) { // 描画結果が不変なら renderer を呼ばず最新の state だけ採用する（react-sdk と同じ）。 polygonManager.registerEntity(PolygonEntity(state = state, polygon = prevEntity.polygon)) @@ -162,7 +163,9 @@ abstract class PolygonController( .find(position) override fun destroy() { - // No native resources to clean up for polygons + // A destroyed controller can still be referenced (by its map controller, + // or a state that has not let go yet); without this it keeps every entity. + polygonManager.clear() } override fun has(id: String): Boolean = polygonManager.hasEntity(id) diff --git a/src/main/java/com/mapconductor/core/polyline/PolylineController.kt b/src/main/java/com/mapconductor/core/polyline/PolylineController.kt index d5080cd..361cda6 100644 --- a/src/main/java/com/mapconductor/core/polyline/PolylineController.kt +++ b/src/main/java/com/mapconductor/core/polyline/PolylineController.kt @@ -39,7 +39,8 @@ abstract class PolylineController( data.forEach { state -> if (previous.contains(state.id)) { - val prevEntity = polylineManager.getEntity(state.id)!! + // Gone only if destroy() cleared the manager meanwhile; nothing left to update. + val prevEntity = polylineManager.getEntity(state.id) ?: return@withPermit if (state.fingerPrint() == prevEntity.fingerPrint) { // 描画結果が不変なら renderer を呼ばず最新の state だけ採用する（react-sdk と同じ）。 polylineManager.registerEntity(PolylineEntity(state = state, polyline = prevEntity.polyline)) @@ -169,7 +170,9 @@ abstract class PolylineController( } override fun destroy() { - // No native resources to clean up for polylines + // A destroyed controller can still be referenced (by its map controller, + // or a state that has not let go yet); without this it keeps every entity. + polylineManager.clear() } override fun has(id: String): Boolean = polylineManager.hasEntity(id) diff --git a/src/main/java/com/mapconductor/core/raster/RasterLayerController.kt b/src/main/java/com/mapconductor/core/raster/RasterLayerController.kt index 4b3d831..f6b3631 100644 --- a/src/main/java/com/mapconductor/core/raster/RasterLayerController.kt +++ b/src/main/java/com/mapconductor/core/raster/RasterLayerController.kt @@ -282,7 +282,9 @@ abstract class RasterLayerController( } override fun destroy() { - // No native resources to clean up + // A destroyed controller can still be referenced (by its map controller, + // or a state that has not let go yet); without this it keeps every entity. + rasterLayerManager.clear() } override fun has(id: String): Boolean = rasterLayerManager.hasEntity(id) diff --git a/src/main/java/com/mapconductor/core/tileserver/ScratchPool.kt b/src/main/java/com/mapconductor/core/tileserver/ScratchPool.kt new file mode 100644 index 0000000..dcfd778 --- /dev/null +++ b/src/main/java/com/mapconductor/core/tileserver/ScratchPool.kt @@ -0,0 +1,49 @@ +package com.mapconductor.core.tileserver + +/** + * A few reusable scratch objects (pixel buffers, bitmaps, output streams) for + * code that renders tiles on threads it does not own. + * + * Use this instead of a `ThreadLocal`. Tile callbacks run on the map SDK's + * threads, and some SDKs start new ones for every map and never stop them: + * Google Maps keeps an `androidmapsapi-tileoverlay` pool of three live threads + * per map it ever created. A thread-local scratch pins one object on each such + * thread for good, so every map switch added a few megabytes that nothing + * could free. A pool holds at most [capacity] objects whatever the threads do. + * + * [acquire] hands out a pooled object or null (the caller makes a fresh one); + * [release] takes it back, or drops it when the pool is full. + */ +internal class ScratchPool( + private val capacity: Int = DEFAULT_CAPACITY, +) { + private val items = ArrayDeque(capacity) + + /** A pooled object that [accept] takes, or null. Others are dropped. */ + fun acquire(accept: (T) -> Boolean = { true }): T? { + synchronized(items) { + while (items.isNotEmpty()) { + val item = items.removeLast() + if (accept(item)) return item + } + } + return null + } + + /** Returns [item]. False if the pool was full and did not keep it. */ + fun release(item: T): Boolean = + synchronized(items) { + if (items.size >= capacity) return false + items.addLast(item) + true + } + + /** Pooled objects right now. For tests. */ + val size: Int + get() = synchronized(items) { items.size } + + companion object { + /** About how many tiles are drawn at the same time. */ + val DEFAULT_CAPACITY: Int = Runtime.getRuntime().availableProcessors().coerceIn(2, 8) + } +} diff --git a/src/main/java/com/mapconductor/core/tileserver/TilePngEncoder.kt b/src/main/java/com/mapconductor/core/tileserver/TilePngEncoder.kt index 7c09b6a..4bea4ca 100644 --- a/src/main/java/com/mapconductor/core/tileserver/TilePngEncoder.kt +++ b/src/main/java/com/mapconductor/core/tileserver/TilePngEncoder.kt @@ -23,13 +23,15 @@ import android.graphics.Bitmap */ object TilePngEncoder { /** - * Scratch pixel buffer, one per calling thread. + * Scratch pixel buffers, shared by the calling threads. * - * Tile rendering runs concurrently across the tile server's threads, and a - * tile at this SDK's densities is around 7 MB of RGBA — allocating that per - * call would cost more than the encoding it is meant to speed up. + * Tile rendering runs concurrently across several threads, and a tile at + * this SDK's densities is around 7 MB of RGBA — allocating that per call + * would cost more than the encoding it is meant to speed up. Not one per + * thread: the callers include map SDK threads that outlive their map (see + * [ScratchPool]), and each would keep its buffer for good. */ - private val scratch = ThreadLocal() + private val scratch = ScratchPool() /** False where the native library is missing for this device's ABI. */ val isAvailable: Boolean = @@ -55,16 +57,17 @@ object TilePngEncoder { if (width <= 0 || height <= 0) return null val needed = width * height * 4 - var buffer = scratch.get() - if (buffer == null || buffer.capacity() < needed) { - buffer = ByteBuffer.allocateDirect(needed).order(ByteOrder.nativeOrder()) - scratch.set(buffer) + val buffer = + scratch.acquire { it.capacity() >= needed } + ?: ByteBuffer.allocateDirect(needed).order(ByteOrder.nativeOrder()) + try { + buffer.clear() + bitmap.copyPixelsToBuffer(buffer) + buffer.rewind() + return encode(buffer, width, height, premultiplied = bitmap.isPremultiplied) + } finally { + scratch.release(buffer) } - buffer.clear() - bitmap.copyPixelsToBuffer(buffer) - buffer.rewind() - - return encode(buffer, width, height, premultiplied = bitmap.isPremultiplied) } /** diff --git a/src/test/java/com/mapconductor/core/map/MapViewStateBaseTest.kt b/src/test/java/com/mapconductor/core/map/MapViewStateBaseTest.kt index 19a5e8e..73d519c 100644 --- a/src/test/java/com/mapconductor/core/map/MapViewStateBaseTest.kt +++ b/src/test/java/com/mapconductor/core/map/MapViewStateBaseTest.kt @@ -258,4 +258,31 @@ class MapViewStateBaseTest { assertTrue(controller.moved.isEmpty()) assertNull(state.getMapViewHolder()) } + + @Test + fun `破棄したコントローラを渡すと手放す`() { + // アプリが state を持ち続けても、壊した地図のコントローラ（と全オーバーレイ）を + // 抱えたままにしない。 + val controller = FakeController() + val state = TestState().apply { attach(controller) } + + state.detachController(controller) + + assertNull(state.getMapViewHolder()) + } + + @Test + fun `別のコントローラが接続済みなら古いものを渡しても外さない`() { + val old = FakeController() + val current = FakeController() + val state = TestState().apply { attach(old) } + state.attach(current) + + state.detachController(old) + + assertSame(current.holder, state.getMapViewHolder()) + current.moved.clear() + state.moveCameraTo(cameraAt(1.0, 2.0), durationMillis = 0) + assertEquals(1, current.moved.size) + } } diff --git a/src/test/java/com/mapconductor/core/tileserver/ScratchPoolTest.kt b/src/test/java/com/mapconductor/core/tileserver/ScratchPoolTest.kt new file mode 100644 index 0000000..61d2e5b --- /dev/null +++ b/src/test/java/com/mapconductor/core/tileserver/ScratchPoolTest.kt @@ -0,0 +1,42 @@ +package com.mapconductor.core.tileserver + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test + +class ScratchPoolTest { + @Test + fun `holds no more than its capacity, however many threads used it`() { + // A thread-local kept one per thread, and map SDK threads that never stop + // (Google's tile-overlay pool, three per map) kept theirs for good. + val pool = ScratchPool(capacity = 2) + val released = (1..10).map { pool.release(ByteArray(1)) } + + assertEquals(listOf(true, true) + List(8) { false }, released) + assertEquals(2, pool.size) + } + + @Test + fun `hands back what was released`() { + val pool = ScratchPool(capacity = 2) + val item = ByteArray(4) + pool.release(item) + + assertSame(item, pool.acquire()) + assertNull(pool.acquire()) + } + + @Test + fun `drops pooled objects the caller cannot use`() { + val pool = ScratchPool(capacity = 2) + pool.release(ByteArray(4)) + + assertNull(pool.acquire { it.size >= 8 }) + assertEquals(0, pool.size) + assertTrue(pool.release(ByteArray(8))) + assertFalse(pool.acquire { it.size >= 8 } == null) + } +}