fix(Android): drop zero frames in release mode #607
Conversation
| @@ -242,11 +244,11 @@ fun HomeMapView( | |||
| LaunchedEffect(railRouteShapes, globalResponse, globalMapData) { | |||
| viewModel.refreshRouteLineData(now) | |||
| } | |||
| LaunchedEffect(railRouteLineData) { | |||
| LaunchedOnDispatcherEffect(Dispatchers.Default, railRouteLineData) { | |||
There was a problem hiding this comment.
I like the way you broke out reusable helpers, but I'm concerned about how we would consistently use these new helpers when appropriate.
Wrapping my head more on coroutines, I found a couple of snippets from this blog post helpful:
a good practice is to use withContext to make sure every function is safe to be called on any Dispatcher including Main — that way the caller never has to think about what thread will be needed to execute the function.
It’s a really good idea to make every suspend function main-safe. If it does anything that touches the disk, network, or even just uses too much CPU, use withContext to make it safe to call from the main thread. This is the pattern that coroutines based libraries like Retrofit and Room follow. If you follow this style throughout your codebase your code will be much simpler and avoid mixing threading concerns with application logic. When followed consistently, coroutines are free to launch on the main thread and make network or database requests with simple code while guaranteeing users won’t see “jank.”
What do you think of pushing the Dispatcher context to a lower level, such as within refreshRouteLineSource and refreshStopFeatures so that the view doesn't need to worry about thread management?
Aside from making any changes to the implementation here, I think it would be worth adding a note to the PR template to check that expensive computations are run on the Default thread as appropriate.
There was a problem hiding this comment.
That's definitely a more robust approach, good catch.
| @@ -56,7 +58,8 @@ fun NearbyTransitView( | |||
| val (pinnedRoutes, togglePinnedRoute) = managePinnedRoutes() | |||
|
|
|||
| val nearbyWithRealtimeInfo = | |||
| remember( | |||
| rememberOnDispatcher( | |||
There was a problem hiding this comment.
question: Similar to above, I'm concerned about how we'll approach using this consistently, especially where we'll want rememberSaveable as well. Could the use of Dispatchers.Default be pushed to a lower level?
There was a problem hiding this comment.
Making withRealtimeInfo suspend in shared is a mess I'd rather not touch right now - it implies that StopDetailsDepartures.fromData needs to also be suspend, but then code generated by SKIE fails to compile due to ambiguity between shared as in shared.framework and shared as in StopDetailsDepartures.Companion.shared, and fixing that would require replacing every import shared in iosApp/. That might fix at least some of our iOS UI thread jank, though, so we should probably do it at some point.
Android has a set of thread annotations to keep track of functions which must be called from the UI thread or from a worker thread, but it looks like it doesn't actually check those in a way that would let us error on calling withRealtimeInfo from the UI thread.
There was a problem hiding this comment.
SKIE bug reported. I'm not sure SKIE will be able to fix this, so we may wind up needing to rename the framework (not sure if Shared or MbtaGoShared makes more sense) and s/import shared/import Shared/ in iosApp/; if we'd be neutral-to-positive on doing that anyway, I wouldn't mind going ahead and picking that up, but either way this PR should be set to go now.
There was a problem hiding this comment.
Nice catch on the SKIE bug! I don't think it is too pressing to pick up since it would improve iOS performance which isn't a major issue right now, but worth making a ticket & bringing up at the next refinement!
androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/MapLayerManager.kt
Outdated
Show resolved
Hide resolved
Summary
Ticket: 🤖 | Nearby | Sheet / navigation, tangentially
#605 mentions that the animation doesn't play because frames are lost. This fixes that issue by running some of our most CPU-intensive computations on a background thread.
iOS
android
Testing
Manually verified that Logcat shows zero Choreographer warnings about skipped frames in release mode. (Some are still shown in debug mode on launch, but from the profiler it looks like the code running in those slow frames is mostly not ours.)