Skip to content

Commit

Permalink
Optimize ImageRequest to avoid creating new Extras or Maps if possibl…
Browse files Browse the repository at this point in the history 
…e. (#2390)

* Optimize ImageRequest to avoid creating new Extras or collections after newBuilder.

* Cast to MutableMap.
  • Loading branch information
@colinrtwhite
colinrtwhite authored Jul 22, 2024
1 parent 1c25c6f commit 60e9892
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 26 deletions.
40 changes: 40 additions & 0 deletions coil-core/src/androidUnitTest/kotlin/coil3/request/ImageRequestTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,4 +147,44 @@ class ImageRequestTest : RobolectricTest() {
assertEquals(Size(100, 100), request3.sizeResolver.size())
assertEquals(Size(100, 100), request4.sizeResolver.size())
}

@Test
fun `memoryCacheKeyExtras and extras are reused`() {
val extraKey = Extras.Key(default = "test")
val extraValue = "something"
val memoryCacheKeyExtras = mapOf("one" to "memory_cache_key")
val request1 = ImageRequest.Builder(context)
.apply { extras[extraKey] = extraValue }
.memoryCacheKeyExtras(memoryCacheKeyExtras)
.build()
val request2 = request1.newBuilder().build()

assertSame(request1.memoryCacheKeyExtras, request2.memoryCacheKeyExtras)
assertSame(request1.extras, request2.extras)
}

@Test
fun `memoryCacheKeyExtras and extras are not reused if modified`() {
val extraKey = Extras.Key(default = "test")
val extraValue1 = "something"
val extraValue2 = "something_else"
val request1 = ImageRequest.Builder(context)
.apply { extras[extraKey] = extraValue1 }
.memoryCacheKeyExtra("one", "memory_cache_key1")
.build()
val request2 = request1.newBuilder()
.apply { extras[extraKey] = extraValue2 }
.memoryCacheKeyExtra("two", "memory_cache_key2")
.build()

assertEquals("memory_cache_key1", request1.memoryCacheKeyExtras["one"])
assertEquals(null, request1.memoryCacheKeyExtras["two"])
assertEquals("memory_cache_key1", request2.memoryCacheKeyExtras["one"])
assertEquals("memory_cache_key2", request2.memoryCacheKeyExtras["two"])
assertNotEquals(request1.memoryCacheKeyExtras, request2.memoryCacheKeyExtras)

assertEquals(extraValue1, request1.extras[extraKey])
assertEquals(extraValue2, request2.extras[extraKey])
assertNotEquals(request1.extras, request2.extras)
}
}
61 changes: 35 additions & 26 deletions coil-core/src/commonMain/kotlin/coil3/request/ImageRequest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import coil3.annotation.Poko
import coil3.decode.Decoder
import coil3.fetch.Fetcher
import coil3.memory.MemoryCache
import coil3.orEmpty
import coil3.request.ImageRequest.Builder
import coil3.size.Dimension
import coil3.size.Precision
Expand All @@ -20,6 +19,7 @@ import coil3.target.Target
import coil3.util.EMPTY_IMAGE_FACTORY
import coil3.util.defaultFileSystem
import coil3.util.ioCoroutineDispatcher
import coil3.util.toImmutableMap
import kotlin.coroutines.CoroutineContext
import kotlin.coroutines.EmptyCoroutineContext
import kotlin.jvm.JvmField
Expand Down Expand Up @@ -263,17 +263,25 @@ class ImageRequest private constructor(
}
}

@Suppress("UNCHECKED_CAST")
class Builder {
private val context: PlatformContext
private var defaults: Defaults
private var data: Any?
private var target: Target?
private var listener: Listener?
private var memoryCacheKey: String?
private var lazyMemoryCacheKeyExtras: MutableMap<String, String>?
private var memoryCacheKeyExtrasAreMutable = false
private var lazyMemoryCacheKeyExtras: Any
private val memoryCacheKeyExtras: MutableMap<String, String>
get() = lazyMemoryCacheKeyExtras
?: mutableMapOf<String, String>().also { lazyMemoryCacheKeyExtras = it }
get() = if (memoryCacheKeyExtrasAreMutable) {
lazyMemoryCacheKeyExtras
} else {
(lazyMemoryCacheKeyExtras as Map<*, *>).toMutableMap().also {
lazyMemoryCacheKeyExtras = it
memoryCacheKeyExtrasAreMutable = true
}
} as MutableMap<String, String>
private var diskCacheKey: String?
private var fileSystem: FileSystem?
private var fetcherFactory: Pair<Fetcher.Factory<*>, KClass<*>>?
Expand All @@ -291,9 +299,13 @@ class ImageRequest private constructor(
private var sizeResolver: SizeResolver?
private var scale: Scale?
private var precision: Precision?
private var lazyExtras: Extras.Builder?
private var lazyExtras: Any
val extras: Extras.Builder
get() = lazyExtras ?: Extras.Builder().also { lazyExtras = it }
get() = when (val extras = lazyExtras) {
is Extras.Builder -> extras
is Extras -> extras.newBuilder().also { lazyExtras = it }
else -> throw AssertionError()
}

constructor(context: PlatformContext) {
this.context = context
Expand All @@ -302,7 +314,7 @@ class ImageRequest private constructor(
target = null
listener = null
memoryCacheKey = null
lazyMemoryCacheKeyExtras = null
lazyMemoryCacheKeyExtras = emptyMap<String, String>()
diskCacheKey = null
fileSystem = null
fetcherFactory = null
Expand All @@ -320,7 +332,7 @@ class ImageRequest private constructor(
sizeResolver = null
scale = null
precision = null
lazyExtras = null
lazyExtras = Extras.EMPTY
}

@JvmOverloads
Expand All @@ -331,11 +343,7 @@ class ImageRequest private constructor(
target = request.target
listener = request.listener
memoryCacheKey = request.memoryCacheKey
lazyMemoryCacheKeyExtras = if (request.memoryCacheKeyExtras.isEmpty()) {
null
} else {
request.memoryCacheKeyExtras.toMutableMap()
}
lazyMemoryCacheKeyExtras = request.memoryCacheKeyExtras
diskCacheKey = request.diskCacheKey
fileSystem = request.defined.fileSystem
fetcherFactory = request.fetcherFactory
Expand All @@ -353,11 +361,7 @@ class ImageRequest private constructor(
sizeResolver = request.defined.sizeResolver
scale = request.defined.scale
precision = request.defined.precision
lazyExtras = if (request.extras.asMap().isEmpty()) {
null
} else {
request.extras.newBuilder()
}
lazyExtras = request.extras
}

/**
Expand Down Expand Up @@ -410,11 +414,8 @@ class ImageRequest private constructor(
* Set extra values to be added to this image request's memory cache key.
*/
fun memoryCacheKeyExtras(extras: Map<String, String>) = apply {
this.lazyMemoryCacheKeyExtras = if (extras.isEmpty()) {
null
} else {
extras.toMutableMap()
}
this.lazyMemoryCacheKeyExtras = extras.toMutableMap()
this.memoryCacheKeyExtrasAreMutable = true
}

/**
Expand All @@ -424,7 +425,7 @@ class ImageRequest private constructor(
if (value != null) {
this.memoryCacheKeyExtras[key] = value
} else {
this.lazyMemoryCacheKeyExtras?.remove(key)
this.memoryCacheKeyExtras.remove(key)
}
}

Expand Down Expand Up @@ -665,7 +666,11 @@ class ImageRequest private constructor(
target = target,
listener = listener,
memoryCacheKey = memoryCacheKey,
memoryCacheKeyExtras = lazyMemoryCacheKeyExtras?.toMap().orEmpty(),
memoryCacheKeyExtras = if (memoryCacheKeyExtrasAreMutable) {
(lazyMemoryCacheKeyExtras as MutableMap<*, *>).toImmutableMap()
} else {
lazyMemoryCacheKeyExtras
} as Map<String, String>,
diskCacheKey = diskCacheKey,
fileSystem = fileSystem ?: defaults.fileSystem,
fetcherFactory = fetcherFactory,
Expand All @@ -683,7 +688,11 @@ class ImageRequest private constructor(
sizeResolver = sizeResolver ?: defaults.sizeResolver,
scale = scale ?: defaults.scale,
precision = precision ?: defaults.precision,
extras = lazyExtras?.build().orEmpty(),
extras = when (val extras = lazyExtras) {
is Extras.Builder -> extras.build()
is Extras -> extras
else -> throw AssertionError()
},
defined = Defined(
fileSystem = fileSystem,
interceptorCoroutineContext = interceptorCoroutineContext,
Expand Down

0 comments on commit 60e9892

Please sign in to comment.