Improve RealImageLoader.execute to properly call async request #2205
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| package coil.benchmark | ||
|
|
||
| import androidx.benchmark.macro.ExperimentalMetricApi | ||
| import androidx.benchmark.macro.TraceMetric | ||
| import androidx.benchmark.perfetto.ExperimentalPerfettoTraceProcessorApi | ||
| import androidx.benchmark.perfetto.PerfettoTraceProcessor | ||
|
|
||
| /** | ||
| * TraceSectionMetric to give average/sum in microseconds measurements. | ||
| */ | ||
| @OptIn(ExperimentalMetricApi::class) | ||
| class MicrosTraceSectionMetric( | ||
| private val sectionName: String, | ||
| private vararg val mode: Mode, | ||
| private val label: String = sectionName, | ||
| private val targetPackageOnly: Boolean = true, | ||
| ) : TraceMetric() { | ||
|
|
||
| enum class Mode { | ||
| Sum, | ||
| Average | ||
| } | ||
|
|
||
| @ExperimentalPerfettoTraceProcessorApi | ||
| @Suppress("RestrictedApi") | ||
| override fun getResult( | ||
| captureInfo: CaptureInfo, | ||
| traceSession: PerfettoTraceProcessor.Session, | ||
| ): List<Measurement> { | ||
| val slices = traceSession.querySlices( | ||
| sectionName, | ||
| packageName = if (targetPackageOnly) captureInfo.targetPackageName else null, | ||
| ) | ||
|
|
||
| return mode.flatMap { m -> | ||
| when (m) { | ||
| Mode.Sum -> listOf( | ||
| Measurement( | ||
| name = sectionName + "_µs", | ||
| // note, this duration assumes non-reentrant slices | ||
| data = slices.sumOf { it.dur } / 1_000.0, | ||
| ), | ||
| Measurement( | ||
| name = sectionName + "Count", | ||
| data = slices.size.toDouble(), | ||
| ), | ||
| ) | ||
|
|
||
| Mode.Average -> listOf( | ||
| Measurement( | ||
| name = label + "Average_µs", | ||
| data = slices.sumOf { it.dur } / 1_000.0 / slices.size, | ||
| ), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| package coil.benchmark | ||
|
|
||
| import android.graphics.Point | ||
| import android.os.Build | ||
| import androidx.annotation.RequiresApi | ||
| import androidx.benchmark.macro.BaselineProfileMode | ||
| import androidx.benchmark.macro.CompilationMode | ||
| import androidx.benchmark.macro.FrameTimingMetric | ||
| import androidx.benchmark.macro.StartupMode | ||
| import androidx.benchmark.macro.StartupTimingMetric | ||
| import androidx.benchmark.macro.junit4.MacrobenchmarkRule | ||
| import androidx.test.ext.junit.runners.AndroidJUnit4 | ||
| import androidx.test.uiautomator.By | ||
| import coil.benchmark.BuildConfig.PROJECT | ||
| import coil.benchmark.MicrosTraceSectionMetric.Mode.Average | ||
| import coil.benchmark.MicrosTraceSectionMetric.Mode.Sum | ||
| import org.junit.Rule | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
|
|
||
| @RunWith(AndroidJUnit4::class) | ||
| class ScrollBenchmark { | ||
|
|
||
| @get:Rule | ||
| val benchmarkRule = MacrobenchmarkRule() | ||
|
|
||
| @RequiresApi(Build.VERSION_CODES.N) | ||
| @Test | ||
| fun baselineProfile() { | ||
| benchmark(CompilationMode.Partial(BaselineProfileMode.Require)) | ||
| } | ||
|
|
||
| @Test | ||
| fun fullCompilation() { | ||
| benchmark(CompilationMode.Full()) | ||
| } | ||
|
|
||
| private fun benchmark(compilationMode: CompilationMode) { | ||
| benchmarkRule.measureRepeated( | ||
| packageName = "sample.$PROJECT", | ||
| metrics = listOf( | ||
| FrameTimingMetric(), | ||
| StartupTimingMetric(), | ||
| MicrosTraceSectionMetric( | ||
| "rememberAsyncImagePainter", | ||
| Sum, Average, | ||
| ), | ||
| MicrosTraceSectionMetric( | ||
| "AsyncImagePainter.onRemembered", | ||
| Sum, Average, | ||
| ), | ||
| ), | ||
| iterations = 20, | ||
| startupMode = StartupMode.COLD, | ||
| compilationMode = compilationMode, | ||
| measureBlock = { | ||
| startActivityAndWait() | ||
| Thread.sleep(3_000) | ||
| val list = device.findObject(By.res("list")) | ||
| list.setGestureMargin(device.displayWidth / 5) | ||
| list.drag(Point(list.visibleBounds.centerX(), list.visibleBounds.top)) | ||
| Thread.sleep(300) | ||
| }, | ||
| ) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| package coil.compose | ||
|
|
||
| import android.annotation.SuppressLint | ||
| import android.graphics.drawable.BitmapDrawable | ||
| import android.graphics.drawable.Drawable | ||
| import androidx.compose.foundation.Image | ||
|
|
@@ -9,12 +10,12 @@ import androidx.compose.runtime.Composable | |
| import androidx.compose.runtime.NonRestartableComposable | ||
| import androidx.compose.runtime.RememberObserver | ||
| import androidx.compose.runtime.Stable | ||
| import androidx.compose.runtime.collectAsState | ||
| import androidx.compose.runtime.getValue | ||
| import androidx.compose.runtime.mutableFloatStateOf | ||
| import androidx.compose.runtime.mutableStateOf | ||
| import androidx.compose.runtime.remember | ||
| import androidx.compose.runtime.setValue | ||
| import androidx.compose.ui.geometry.Size | ||
| import androidx.compose.ui.geometry.isUnspecified | ||
| import androidx.compose.ui.graphics.ColorFilter | ||
|
|
@@ -30,6 +31,7 @@ import androidx.compose.ui.graphics.vector.ImageVector | |
| import androidx.compose.ui.layout.ContentScale | ||
| import androidx.compose.ui.platform.LocalInspectionMode | ||
| import androidx.compose.ui.unit.Constraints | ||
| import androidx.compose.ui.util.trace | ||
| import coil.ImageLoader | ||
| import coil.compose.AsyncImagePainter.Companion.DefaultTransform | ||
| import coil.compose.AsyncImagePainter.State | ||
|
|
@@ -197,7 +199,7 @@ private fun rememberAsyncImagePainter( | |
| onState: ((State) -> Unit)?, | ||
| contentScale: ContentScale, | ||
| filterQuality: FilterQuality, | ||
| ): AsyncImagePainter = trace("rememberAsyncImagePainter") { | ||
| val request = requestOf(state.model) | ||
| validateRequest(request) | ||
|
|
||
|
|
@@ -208,7 +210,9 @@ private fun rememberAsyncImagePainter( | |
| painter.filterQuality = filterQuality | ||
| painter.isPreview = LocalInspectionMode.current | ||
| painter.imageLoader = state.imageLoader | ||
| @SuppressLint("StateFlowValueCalledInComposition") | ||
| painter.requestInternal.value = | ||
| request // Update request last so all other properties are up to date. | ||
| painter.onRemembered() // Invoke this manually so `painter.state` is set to `Loading` immediately. | ||
| return painter | ||
| } | ||
|
|
@@ -247,17 +251,19 @@ class AsyncImagePainter internal constructor( | |
| internal var contentScale = ContentScale.Fit | ||
| internal var filterQuality = DefaultFilterQuality | ||
| internal var isPreview = false | ||
| internal val requestInternal = MutableStateFlow(request) | ||
|
There was a problem hiding this comment. Is the a performance benefit to reading request from a There was a problem hiding this comment. Actually, yes, which is why I was trying a different options. When
|
||
|
|
||
| /** The current [AsyncImagePainter.State]. */ | ||
| var state: State by mutableStateOf(State.Empty) | ||
| private set | ||
|
|
||
| /** The current [ImageRequest]. */ | ||
| val request | ||
| @Composable | ||
| get() = requestInternal.collectAsState().value | ||
|
There was a problem hiding this comment. @colinrtwhite what do you think about this change? Technically this changes the API, because now the There was a problem hiding this comment. I'd prefer to keep it as a non-Composable since it's then the same as the other exposed properties ( There was a problem hiding this comment. Depends what strong is. I think with the coroutines optimizations + the async, we can squeeze ~0.2ms for each I don't want to just come and introduce breaking changes, which is why I try to have this discussion. There was a problem hiding this comment. @mlykotom I think 0.2ms is probably a strong enough incentive to change the API. Though unfortunately, we can't make any breaking changes to the 2.x branch. We could make the changes on the 3.x ( |
||
|
|
||
| /** The current [ImageLoader]. */ | ||
| var imageLoader: ImageLoader = imageLoader | ||
|
There was a problem hiding this comment. I think we need to keep this as a There was a problem hiding this comment. I have a feeling it doesn't have to. The readers of this are in the This state is only needed if someone would like to observe |
||
| internal set | ||
|
|
||
| override val intrinsicSize: Size | ||
|
|
@@ -282,9 +288,9 @@ class AsyncImagePainter internal constructor( | |
| } | ||
|
|
||
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| override fun onRemembered() = trace("AsyncImagePainter.onRemembered") { | ||
|
There was a problem hiding this comment. I'm thinking whether the usage of I don't fully understand (yet) the child painter and it's relation to There was a problem hiding this comment. I'm not too sure of Compose's behaviour, but I added this in on The child painter is usually either a There was a problem hiding this comment. I think the current way how it's used it doesn't really need to call this in The I think this could optimized into starting the Anyway, I'll revert this so the API stays the same |
||
| // Short circuit if we're already remembered. | ||
| if (rememberScope != null) return@trace | ||
|
|
||
| // Create a new scope to observe state and execute requests while we're remembered. | ||
| val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate) | ||
|
|
@@ -295,14 +301,14 @@ class AsyncImagePainter internal constructor( | |
|
|
||
| // If we're in inspection mode skip the image request and set the state to loading. | ||
| if (isPreview) { | ||
| val request = requestInternal.value.newBuilder().defaults(imageLoader.defaults).build() | ||
| updateState(State.Loading(request.placeholder?.toPainter())) | ||
| return@trace | ||
| } | ||
|
|
||
| // Observe the current request and execute any emissions. | ||
| scope.launch { | ||
| requestInternal | ||
| .mapLatest { imageLoader.execute(updateRequest(it)).toState() } | ||
|
There was a problem hiding this comment. What if this is actually just a I don't know what's the case for the nested painters. There was a problem hiding this comment. I think we might need to read this as a state, but I can't remember the specifics. If I remember correctly there are cases were There was a problem hiding this comment. Because the |
||
| .collect(::updateState) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this
traceadd any overhead? Is it OK to ship to users?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This trace doesn't add any overhead (~6ns per call) and it's fine to ship to production. Compose adds bunch of custom sections as well. These usually serve for better understanding what's going on.
You can even use these sections from the benchmarks without enabling composition tracing to keep track of how long they take.