Using semaphore rather than synchronizing on generic object. (#7836)

mapbox · Jul 10, 2024 · ae64202 · ae64202
1 parent 9b40acb
commit ae64202
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 18 deletions.
diff --git a/app-mincompile/src/main/java/com/mapbox/navigation/examples/mincompile/DropInUIActivity.kt b/app-mincompile/src/main/java/com/mapbox/navigation/examples/mincompile/DropInUIActivity.kt
@@ -3,8 +3,6 @@ package com.mapbox.navigation.examples.mincompile
 import androidx.appcompat.app.AppCompatActivity
 import android.os.Bundle
 import com.mapbox.navigation.examples.mincompile.databinding.LayoutActivityDropinBinding
-import com.mapbox.navigation.examples.mincompile.databinding.LayoutActivityMainBinding
-import com.mapbox.navigation.examples.mincompile.databinding.LayoutActivityNavigationBinding
 
 class DropInUIActivity : AppCompatActivity() {
     private lateinit var binding: LayoutActivityDropinBinding

diff --git a/...n-base/src/main/java/com/mapbox/navigation/base/internal/route/RouteCompatibilityCache.kt b/...n-base/src/main/java/com/mapbox/navigation/base/internal/route/RouteCompatibilityCache.kt
@@ -1,11 +1,11 @@
 package com.mapbox.navigation.base.internal.route
 
-import android.util.LruCache
 import com.mapbox.api.directions.v5.models.DirectionsRoute
 import com.mapbox.navigation.base.route.NavigationRoute
+import java.util.concurrent.ConcurrentHashMap
 
 /**
- * **Internal** cache object that aims to improve the performance performance of compatibility functions
+ * **Internal** cache object that aims to improve the performance of compatibility functions
  * that transform the [DirectionsRoute] route into a [NavigationRoute].
  *
  * It caches up to 3 randomly created [NavigationRoute] instances,
@@ -15,39 +15,50 @@ import com.mapbox.navigation.base.route.NavigationRoute
  * This should be relatively efficient since we treat `MapboxDirectionsSession` as the source of truth and `MapboxNavigation` will clear it on its `onDestroy` as well.
  */
 object RouteCompatibilityCache {
-    private val directionsSessionCache = mutableListOf<NavigationRoute>()
-    private val creationCache = LruCache<DirectionsRoute, NavigationRoute>(3)
-    private val lock = Any()
+    private const val MAX_CACHE_SIZE = 3
+    private val creationCache =
+        ConcurrentHashMap<DirectionsRoute, NavigationRoutePackage>(MAX_CACHE_SIZE)
 
     /**
      * Use to put a result of a random route creation to cache.
      */
     fun cacheCreationResult(routes: List<NavigationRoute>) {
-        synchronized(lock) {
-            routes.forEach {
-                creationCache.put(it.directionsRoute, it)
-            }
+        routes.forEach {
+            creationCache[it.directionsRoute] = NavigationRoutePackage(it, System.nanoTime())
         }
+        maintainCacheSize()
     }
 
     /**
      * Use to put all routes tracked by `MapboxDirectionsSession` to cache (and clear everything else).
      */
     fun setDirectionsSessionResult(routes: List<NavigationRoute>) {
-        synchronized(lock) {
-            creationCache.evictAll()
-            directionsSessionCache.clear()
-            directionsSessionCache.addAll(routes)
+        creationCache.clear()
+        routes.forEach {
+            creationCache[it.directionsRoute] = NavigationRoutePackage(it, System.nanoTime())
         }
+        maintainCacheSize()
     }
 
     /**
      * Get a cached [NavigationRoute] if there is one that wraps the provided [DirectionsRoute] (based on equality) or `null`.
      */
     fun getFor(directionsRoute: DirectionsRoute): NavigationRoute? {
-        synchronized(lock) {
-            return directionsSessionCache.find { it.directionsRoute == directionsRoute }
-                ?: creationCache.get(directionsRoute)
+        return creationCache[directionsRoute]?.route
+    }
+
+    /**
+     * Sorts the items in the cache by timestamp and removes the oldest items in order to maintain
+     * a size no greater than MAX_CACHE_SIZE.
+     */
+    private fun maintainCacheSize() {
+        if (creationCache.size > MAX_CACHE_SIZE) {
+            val items = creationCache.map {
+                Pair(it.key, it.value)
+            }.sortedBy { it.second.timestamp }
+            val overSize = items.size - MAX_CACHE_SIZE
+            items.take(overSize).forEach { creationCache.remove(it.first) }
         }
     }
+    private data class NavigationRoutePackage(val route: NavigationRoute, val timestamp: Long)
 }
diff --git a/...se/src/test/java/com/mapbox/navigation/base/internal/route/RouteCompatibilityCacheTest.kt b/...se/src/test/java/com/mapbox/navigation/base/internal/route/RouteCompatibilityCacheTest.kt
@@ -98,4 +98,27 @@ class RouteCompatibilityCacheTest {
         assertEquals(navigationRoute1, RouteCompatibilityCache.getFor(directionsRouteMock1))
         assertEquals(navigationRoute2, RouteCompatibilityCache.getFor(directionsRouteMock2))
     }
+
+    @Test
+    fun `cache size does not exceed maximum`() {
+        val route1 = mockk<NavigationRoute> {
+            every { directionsRoute } returns mockk()
+        }
+        val route2 = mockk<NavigationRoute> {
+            every { directionsRoute } returns mockk()
+        }
+        val route3 = mockk<NavigationRoute> {
+            every { directionsRoute } returns mockk()
+        }
+        val route4 = mockk<NavigationRoute> {
+            every { directionsRoute } returns mockk()
+        }
+
+        RouteCompatibilityCache.setDirectionsSessionResult(listOf(route1, route2, route3, route4))
+
+        assertNull(RouteCompatibilityCache.getFor(route1.directionsRoute))
+        assertEquals(route2, RouteCompatibilityCache.getFor(route2.directionsRoute))
+        assertEquals(route3, RouteCompatibilityCache.getFor(route3.directionsRoute))
+        assertEquals(route4, RouteCompatibilityCache.getFor(route4.directionsRoute))
+    }
 }