You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Is your feature request related to a problem? Please describe.
There's been some appetite to perform the memory cache check ahead of the interceptor chain. This is a relatively big behaviour change, but it has a couple benefits:
Interceptor code will run by default on background threads. Most interceptors don't need to intercept access to the memory cache and can safely on run any thread. This also allows interceptors to suspend without worrying about delaying the memory cache check and missing the first frame.
This allows us to avoid overwriting the dispatcher in AsyncImagePainterhere. Instead we can use launch(start = CoroutineStart.UNDISPATCHED). We can also remove our dependency on Dispatchers.Main on non-Android platforms (we still need it for Android when using imageLoader.enqueue).
Describe the solution you'd like
Add an opt in flag to ImageLoader and ImageRequest to perform the mapping, keying, and memory cache check here instead of here.
We'll still need to preserve the option to intercept before the memory cache check as some users might want that.
The text was updated successfully, but these errors were encountered:
Took a spike at using launch(start = CoroutineStart.UNDISPATCHED) instead of Dispatchers.Main.immedate and UNDISPATCHED still causes us to dispatch as we only avoid dispatching "until the next suspension", which is imageLoader.execute. This means we likely need to keep Dispatchers.Main.immedate.
Why don't we perform the memory cache check before calling imageLoader.execute?
This would require exposing a lot of internal Coil logic to call inside AsyncImagePainter. We also then have two places we're checking the memory cache which complicates the design.
This isn't possible as Coil requires a size to compute the MemoryCache.Key and SizeResolver.size is a suspending function, which means launch(start = CoroutineStart.UNDISPATCHED) still wouldn't work.
What does imageLoader.execute actually do asynchronously in the fast path (memory cache hit)?
One idea: use a custom dispatcher, similar to the one Compose uses, that drains the queue right before drawing. This dispatcher would only be used for Coil's own coroutine loading calls, so on most frames the pre-draw drain would be a no-op and on frames where there is work, it would only be image-related work.
Pros:
Consistent behavior between Paparazzi and production.
Maintains FIFO scheduling consistent with normal coroutine use.
Cons:
Depending on what is actually done asynchronously in the image loading pipeline, this might not solve anything (e.g. any switches off the main thread would defeat this).
Potentially introduces an unbounded amount of work during draw, which is the most performance-sensitive phase.
This could be somewhat mitigated by putting an upper bound on how many tasks get ran when draining on pre-draw.
Additional complexity in scary, low-level coroutines code which can have a lot of edge cases.
Is your feature request related to a problem? Please describe.
There's been some appetite to perform the memory cache check ahead of the interceptor chain. This is a relatively big behaviour change, but it has a couple benefits:
Interceptor
code will run by default on background threads. Most interceptors don't need to intercept access to the memory cache and can safely on run any thread. This also allows interceptors tosuspend
without worrying about delaying the memory cache check and missing the first frame.AsyncImagePainter
here. Instead we can uselaunch(start = CoroutineStart.UNDISPATCHED)
. We can also remove our dependency onDispatchers.Main
on non-Android platforms (we still need it for Android when usingimageLoader.enqueue
).Describe the solution you'd like
Add an opt in flag to
ImageLoader
andImageRequest
to perform the mapping, keying, and memory cache check here instead of here.We'll still need to preserve the option to intercept before the memory cache check as some users might want that.
The text was updated successfully, but these errors were encountered: