Some cleanup, mainly for execution time

YoshiRulz · Oct 17, 2022 · 0f3a493 · 0f3a493
1 parent b4aac5c
commit 0f3a493
Show file tree

Hide file tree

Showing 12 changed files with 180 additions and 189 deletions.
diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/Cleaner.kt b/app/src/main/java/de/westnordost/streetcomplete/data/Cleaner.kt
@@ -7,6 +7,7 @@ import de.westnordost.streetcomplete.data.osmnotes.NoteController
 import de.westnordost.streetcomplete.data.quest.QuestTypeRegistry
 import de.westnordost.streetcomplete.util.ktx.format
 import de.westnordost.streetcomplete.util.ktx.nowAsEpochMilliseconds
+import kotlin.system.measureTimeMillis
 
 /** Deletes old unused data in the background */
 class Cleaner(
@@ -15,15 +16,14 @@ class Cleaner(
     private val questTypeRegistry: QuestTypeRegistry
 ) {
     fun clean() {
-        val time = nowAsEpochMilliseconds()
-
-        val oldDataTimestamp = nowAsEpochMilliseconds() - ApplicationConstants.DELETE_OLD_DATA_AFTER
-        noteController.deleteOlderThan(oldDataTimestamp, MAX_DELETE_ELEMENTS)
-        mapDataController.deleteOlderThan(oldDataTimestamp, MAX_DELETE_ELEMENTS)
-        /* do this after cleaning map data and notes, because some metadata rely on map data */
-        questTypeRegistry.forEach { it.deleteMetadataOlderThan(oldDataTimestamp) }
-
-        Log.i(TAG, "Cleaning took ${((nowAsEpochMilliseconds() - time) / 1000.0).format(1)}s")
+        val execTimeMs = measureTimeMillis {
+            val oldDataTimestamp = nowAsEpochMilliseconds() - ApplicationConstants.DELETE_OLD_DATA_AFTER
+            noteController.deleteOlderThan(oldDataTimestamp, MAX_DELETE_ELEMENTS)
+            mapDataController.deleteOlderThan(oldDataTimestamp, MAX_DELETE_ELEMENTS)
+            /* do this after cleaning map data and notes, because some metadata rely on map data */
+            questTypeRegistry.forEach { it.deleteMetadataOlderThan(oldDataTimestamp) }
+        }
+        Log.i(TAG, "Cleaning took ${(execTimeMs / 1000.0).format(1)}s")
     }
 
     companion object {

diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/Preloader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/Preloader.kt
@@ -4,12 +4,12 @@ import android.util.Log
 import de.westnordost.countryboundaries.CountryBoundaries
 import de.westnordost.osmfeatures.FeatureDictionary
 import de.westnordost.streetcomplete.util.ktx.format
-import de.westnordost.streetcomplete.util.ktx.nowAsEpochMilliseconds
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.coroutineScope
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
 import java.util.concurrent.FutureTask
+import kotlin.system.measureTimeMillis
 
 /** Initialize certain singleton classes used elsewhere throughout the app in the background */
 class Preloader(
@@ -18,30 +18,26 @@ class Preloader(
 ) {
 
     suspend fun preload() {
-        val time = nowAsEpochMilliseconds()
-        coroutineScope {
-            // country boundaries are necessary latest for when a quest is opened or on a download
-            launch { preloadCountryBoundaries() }
-            // names dictionary is necessary when displaying an element that has no name or
-            // when downloading the place name quest (etc)
-            launch { preloadFeatureDictionary() }
+        val execTimeMs = measureTimeMillis {
+            coroutineScope {
+                // country boundaries are necessary latest for when a quest is opened or on a download
+                launch { preloadCountryBoundaries() }
+                // names dictionary is necessary when displaying an element that has no name or
+                // when downloading the place name quest (etc)
+                launch { preloadFeatureDictionary() }
+            }
         }
-
-        Log.i(TAG, "Preloading data took ${((nowAsEpochMilliseconds() - time) / 1000.0).format(1)}s")
+        Log.i(TAG, "Preloading data took ${(execTimeMs / 1000.0).format(1)}s")
     }
 
     private suspend fun preloadFeatureDictionary() = withContext(Dispatchers.IO) {
-        val time = nowAsEpochMilliseconds()
-        featuresDictionaryFuture.run()
-        val seconds = (nowAsEpochMilliseconds() - time) / 1000.0
-        Log.i(TAG, "Loaded features dictionary in ${seconds.format(1)}s")
+        val execTimeMs = measureTimeMillis { featuresDictionaryFuture.run() }
+        Log.i(TAG, "Loaded features dictionary in ${(execTimeMs / 1000.0).format(1)}s")
     }
 
     private suspend fun preloadCountryBoundaries() = withContext(Dispatchers.IO) {
-        val time = nowAsEpochMilliseconds()
-        countryBoundariesFuture.run()
-        val seconds = (nowAsEpochMilliseconds() - time) / 1000.0
-        Log.i(TAG, "Loaded country boundaries in ${seconds.format(1)}s")
+        val execTimeMs = measureTimeMillis { countryBoundariesFuture.run() }
+        Log.i(TAG, "Loaded country boundaries in ${(execTimeMs / 1000.0).format(1)}s")
     }
 
     companion object {

diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/download/Downloader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/download/Downloader.kt
@@ -15,7 +15,7 @@ import kotlinx.coroutines.coroutineScope
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
-import kotlin.math.max
+import kotlin.system.measureTimeMillis
 
 /** Downloads all the things */
 class Downloader(
@@ -36,25 +36,23 @@ class Downloader(
         }
         Log.i(TAG, "Starting download ($sqkm km², bbox: $bboxString)")
 
-        val time = nowAsEpochMilliseconds()
-
-        mutex.withLock {
-            coroutineScope {
-                // all downloaders run concurrently
-                launch { notesDownloader.download(bbox) }
-                launch { mapDataDownloader.download(bbox) }
-                launch { mapTilesDownloader.download(bbox) }
+        val execTimeMs = measureTimeMillis {
+            mutex.withLock {
+                coroutineScope {
+                    // all downloaders run concurrently
+                    launch { notesDownloader.download(bbox) }
+                    launch { mapDataDownloader.download(bbox) }
+                    launch { mapTilesDownloader.download(bbox) }
+                }
             }
+            putDownloadedAlready(tiles)
         }
-        putDownloadedAlready(tiles)
-
-        val seconds = (nowAsEpochMilliseconds() - time) / 1000.0
-        Log.i(TAG, "Finished download ($sqkm km², bbox: $bboxString) in ${seconds.format(1)}s")
+        Log.i(TAG, "Finished download ($sqkm km², bbox: $bboxString) in ${(execTimeMs / 1000.0).format(1)}s")
     }
 
     private fun hasDownloadedAlready(tiles: TilesRect): Boolean {
-        val freshTime = ApplicationConstants.REFRESH_DATA_AFTER
-        val ignoreOlderThan = max(0, nowAsEpochMilliseconds() - freshTime)
+        val ignoreOlderThan = (nowAsEpochMilliseconds() - ApplicationConstants.REFRESH_DATA_AFTER)
+            .coerceAtLeast(0L)
         return downloadedTilesDb.get(tiles, ignoreOlderThan).contains(DownloadedTilesType.ALL)
     }
 

diff --git a/...main/java/de/westnordost/streetcomplete/data/download/strategy/AVariableRadiusStrategy.kt b/...main/java/de/westnordost/streetcomplete/data/download/strategy/AVariableRadiusStrategy.kt
@@ -16,7 +16,6 @@ import de.westnordost.streetcomplete.util.math.enclosingBoundingBox
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
 import kotlin.math.PI
-import kotlin.math.max
 import kotlin.math.min
 import kotlin.math.sqrt
 
@@ -79,8 +78,8 @@ abstract class AVariableRadiusStrategy(
 
     /** return if data in the given tiles rect that hasn't been downloaded yet */
     private suspend fun hasMissingDataFor(tilesRect: TilesRect): Boolean {
-        val dataExpirationTime = ApplicationConstants.REFRESH_DATA_AFTER
-        val ignoreOlderThan = max(0, nowAsEpochMilliseconds() - dataExpirationTime)
+        val ignoreOlderThan = (nowAsEpochMilliseconds() - ApplicationConstants.REFRESH_DATA_AFTER)
+            .coerceAtLeast(0L)
         val downloadedTiles =
             withContext(Dispatchers.IO) { downloadedTilesDao.get(tilesRect, ignoreOlderThan) }
         return !downloadedTiles.contains(DownloadedTilesType.ALL)

diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/maptiles/MapTilesDownloader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/maptiles/MapTilesDownloader.kt
@@ -6,7 +6,6 @@ import de.westnordost.streetcomplete.data.download.tiles.enclosingTilesRect
 import de.westnordost.streetcomplete.data.osm.mapdata.BoundingBox
 import de.westnordost.streetcomplete.screens.main.map.VectorTileProvider
 import de.westnordost.streetcomplete.util.ktx.format
-import de.westnordost.streetcomplete.util.ktx.nowAsEpochMilliseconds
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.coroutineScope
 import kotlinx.coroutines.launch
@@ -21,6 +20,7 @@ import okhttp3.Response
 import okhttp3.internal.Version
 import java.io.IOException
 import kotlin.coroutines.resume
+import kotlin.system.measureTimeMillis
 
 class MapTilesDownloader(
     private val vectorTileProvider: VectorTileProvider,
@@ -35,30 +35,29 @@ class MapTilesDownloader(
         var failureCount = 0
         var downloadedSize = 0
         var cachedSize = 0
-        val time = nowAsEpochMilliseconds()
-
-        coroutineScope {
-            for (tile in getDownloadTileSequence(bbox)) {
-                launch {
-                    val result = try {
-                        downloadTile(tile.zoom, tile.x, tile.y)
-                    } catch (e: Exception) {
-                        DownloadFailure
-                    }
-                    ++tileCount
-                    when (result) {
-                        is DownloadFailure -> ++failureCount
-                        is DownloadSuccess -> {
-                            if (result.alreadyCached) cachedSize += result.size
-                            else downloadedSize += result.size
+        val execTimeMs = measureTimeMillis {
+            coroutineScope {
+                for (tile in getDownloadTileSequence(bbox)) {
+                    launch {
+                        val result = try {
+                            downloadTile(tile.zoom, tile.x, tile.y)
+                        } catch (e: Exception) {
+                            DownloadFailure
+                        }
+                        ++tileCount
+                        when (result) {
+                            is DownloadFailure -> ++failureCount
+                            is DownloadSuccess -> {
+                                if (result.alreadyCached) cachedSize += result.size
+                                else downloadedSize += result.size
+                            }
                         }
                     }
                 }
             }
         }
-        val seconds = (nowAsEpochMilliseconds() - time) / 1000.0
         val failureText = if (failureCount > 0) ". $failureCount tiles failed to download" else ""
-        Log.i(TAG, "Downloaded $tileCount tiles (${downloadedSize / 1000}kB downloaded, ${cachedSize / 1000}kB already cached) in ${seconds.format(1)}s$failureText")
+        Log.i(TAG, "Downloaded $tileCount tiles (${downloadedSize / 1000}kB downloaded, ${cachedSize / 1000}kB already cached) in ${(execTimeMs / 1000.0).format(1)}s$failureText")
     }
 
     private fun getDownloadTileSequence(bbox: BoundingBox): Sequence<Tile> =

diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataController.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataController.kt
@@ -7,8 +7,9 @@ import de.westnordost.streetcomplete.data.osm.geometry.ElementGeometryCreator
 import de.westnordost.streetcomplete.data.osm.geometry.ElementGeometryDao
 import de.westnordost.streetcomplete.data.osm.geometry.ElementGeometryEntry
 import de.westnordost.streetcomplete.util.ktx.format
-import de.westnordost.streetcomplete.util.ktx.nowAsEpochMilliseconds
 import java.util.concurrent.CopyOnWriteArrayList
+import kotlin.time.ExperimentalTime
+import kotlin.time.measureTimedValue
 
 /** Controller to access element data and its geometry and handle updates to it (from OSM API) */
 class MapDataController internal constructor(
@@ -52,37 +53,39 @@ class MapDataController internal constructor(
 
     /** update element data with [mapData] in the given [bbox] (fresh data from the OSM API has been
      *  downloaded) */
+    @OptIn(ExperimentalTime::class)
     fun putAllForBBox(bbox: BoundingBox, mapData: MutableMapData) {
-        val time = nowAsEpochMilliseconds()
-
-        val oldElementKeys: Set<ElementKey>
-        val geometryEntries: Collection<ElementGeometryEntry>
-        synchronized(this) {
-            // for incompletely downloaded relations, complete the map data (as far as possible) with
-            // local data, i.e. with local nodes and ways (still) in local storage
-            completeMapData(mapData)
+        val (pair, execDuration) = measureTimedValue {
+            val oldElementKeys: Set<ElementKey>
+            val geometryEntries: Collection<ElementGeometryEntry>
+            synchronized(this) {
+                // for incompletely downloaded relations, complete the map data (as far as possible) with
+                // local data, i.e. with local nodes and ways (still) in local storage
+                completeMapData(mapData)
+
+                geometryEntries = createGeometries(mapData, mapData)
+
+                // don't use cache here, because if not everything is already cached, db call will be faster
+                oldElementKeys = elementDB.getAllKeys(mapData.boundingBox!!).toMutableSet()
+                for (element in mapData) {
+                    oldElementKeys.remove(ElementKey(element.type, element.id))
+                }
 
-            geometryEntries = createGeometries(mapData, mapData)
+                // for the cache, use bbox and not mapData.boundingBox because the latter is padded,
+                // see comment for QUEST_FILTER_PADDING
+                cache.update(oldElementKeys, mapData, geometryEntries, bbox)
 
-            // don't use cache here, because if not everything is already cached, db call will be faster
-            oldElementKeys = elementDB.getAllKeys(mapData.boundingBox!!).toMutableSet()
-            for (element in mapData) {
-                oldElementKeys.remove(ElementKey(element.type, element.id))
+                elementDB.deleteAll(oldElementKeys)
+                geometryDB.deleteAll(oldElementKeys)
+                geometryDB.putAll(geometryEntries)
+                elementDB.putAll(mapData)
             }
-
-            // for the cache, use bbox and not mapData.boundingBox because the latter is padded,
-            // see comment for QUEST_FILTER_PADDING
-            cache.update(oldElementKeys, mapData, geometryEntries, bbox)
-
-            elementDB.deleteAll(oldElementKeys)
-            geometryDB.deleteAll(oldElementKeys)
-            geometryDB.putAll(geometryEntries)
-            elementDB.putAll(mapData)
+            Pair(geometryEntries, oldElementKeys)
         }
-
+        val (geometryEntries, oldElementKeys) = pair
         Log.i(TAG,
             "Persisted ${geometryEntries.size} and deleted ${oldElementKeys.size} elements and geometries" +
-            " in ${((nowAsEpochMilliseconds() - time) / 1000.0).format(1)}s"
+            " in ${(execDuration.inWholeMilliseconds / 1000.0).format(1)}s"
         )
 
         val mapDataWithGeometry = MutableMapDataWithGeometry(mapData, geometryEntries)
@@ -166,10 +169,10 @@ class MapDataController internal constructor(
 
     fun getGeometries(keys: Collection<ElementKey>): List<ElementGeometryEntry> = cache.getGeometries(keys, geometryDB::getAllEntries)
 
+    @OptIn(ExperimentalTime::class)
     fun getMapDataWithGeometry(bbox: BoundingBox): MutableMapDataWithGeometry {
-        val time = nowAsEpochMilliseconds()
-        val result = cache.getMapDataWithGeometry(bbox)
-        Log.i(TAG, "Fetched ${result.size} elements and geometries in ${nowAsEpochMilliseconds() - time}ms")
+        val (result, execDuration) = measureTimedValue { cache.getMapDataWithGeometry(bbox) }
+        Log.i(TAG, "Fetched ${result.size} elements and geometries in ${execDuration.inWholeMilliseconds}ms")
 
         return result
     }

diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataDownloader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataDownloader.kt
@@ -4,29 +4,30 @@ import android.util.Log
 import de.westnordost.streetcomplete.ApplicationConstants
 import de.westnordost.streetcomplete.data.download.QueryTooBigException
 import de.westnordost.streetcomplete.util.ktx.format
-import de.westnordost.streetcomplete.util.ktx.nowAsEpochMilliseconds
 import de.westnordost.streetcomplete.util.math.enlargedBy
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
 import kotlinx.coroutines.yield
+import kotlin.time.ExperimentalTime
+import kotlin.time.measureTimedValue
 
 /** Takes care of downloading all note and osm quests */
 class MapDataDownloader(
     private val mapDataApi: MapDataApi,
     private val mapDataController: MapDataController
 ) {
+    @OptIn(ExperimentalTime::class)
     suspend fun download(bbox: BoundingBox) = withContext(Dispatchers.IO) {
-        val time = nowAsEpochMilliseconds()
-
-        val mapData = MutableMapData()
-        val expandedBBox = bbox.enlargedBy(ApplicationConstants.QUEST_FILTER_PADDING)
-        getMapAndHandleTooBigQuery(expandedBBox, mapData)
-        /* The map data might be filled with several bboxes one after another if the download is
-           split up in several, so lets set the bbox back to the bbox of the complete download */
-        mapData.boundingBox = expandedBBox
-
-        val seconds = (nowAsEpochMilliseconds() - time) / 1000.0
-        Log.i(TAG, "Downloaded ${mapData.nodes.size} nodes, ${mapData.ways.size} ways and ${mapData.relations.size} relations in ${seconds.format(1)}s")
+        val (mapData, execDuration) = measureTimedValue {
+            MutableMapData().also { mapData ->
+                val expandedBBox = bbox.enlargedBy(ApplicationConstants.QUEST_FILTER_PADDING)
+                getMapAndHandleTooBigQuery(expandedBBox, mapData)
+                /* The map data might be filled with several bboxes one after another if the download is
+                   split up in several, so lets set the bbox back to the bbox of the complete download */
+                mapData.boundingBox = expandedBBox
+            }
+        }
+        Log.i(TAG, "Downloaded ${mapData.nodes.size} nodes, ${mapData.ways.size} ways and ${mapData.relations.size} relations in ${(execDuration.inWholeMilliseconds / 1000.0).format(1)}s")
 
         yield()