Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider supporting a flag to move the memory cache check ahead of the interceptor chain. #2670

Open
colinrtwhite opened this issue Nov 11, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@colinrtwhite
Copy link
Member

colinrtwhite commented Nov 11, 2024

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 AsyncImagePainter here. 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.

@colinrtwhite colinrtwhite added the enhancement New feature or request label Nov 11, 2024
@colinrtwhite
Copy link
Member Author

colinrtwhite commented Nov 16, 2024

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?

  1. 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.
  2. 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.
Screen_recording_20241115_183033.webm

@zach-klippenstein
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants