Avoid unnecessary initialization in nested lazy initializers #15279
Comments
mcomella
added this to Needs prioritization
in Performance, front-end roadmap
via automation
|
Baron-Severin and I had an interesting discussion: with nested lazy initialization, we can later get a performance issue where, say we're moving through a fragment transaction and trigger a cascading initialization of many components, causing us to drop frames. Given this can be triggered by random code paths, we may not notice this in local development but our users may notice it. This is no reason not to fix this bug – being in an indeliberate half lazy state is just as bad if not worse – but we should consider a solution for this in the future – I filed https://jira.mozilla.com/browse/FXP-1358 to consider implementing this in the future. |
MarcLeclair
moved this from Needs prioritization
to Backlog (prioritized)
in Performance, front-end roadmap
mcomella
moved this from Backlog (prioritized)
to In progress
in Performance, front-end roadmap
|
I wrote up a document with my proposed solution: https://docs.google.com/document/d/12O0elEeo5zOBqFwWJ0mZmh4L2d-UNZrnQe5ZrPdj1_o/edit#heading=h.kvxbynlqzbyf I wanted to get feedback before landing it because it affects how the fenix team interacts with components. |
I want to count the number of components we initialize before we start doing recursive init protection to see how much of an impact the recursive init protection actually makes. |
…t groups. By component groups, I mean I applied this to any class with the class kdoc, "Component group for...". There are a few instances of lazy we had to keep using the old API to avoid having to update constructor arguments.
…-in API use. By having LazyMonitored implement Lazy, we can continue to pass these values directly into the ac APIs that require Lazy references. For some reason, implementing `Lazy.value` can replace `operator fun getValue` required for delegates.
…APIs.
They're currently lazy { lazy { value } }. Accessing `lazy.value`
directly allows us to make it lazy { value }. This should be more
performant and prevents us from double-counting these components.
…t groups. By component groups, I mean I applied this to any class with the class kdoc, "Component group for...". There are a few instances of lazy we had to keep using the old API to avoid having to update constructor arguments.
…-in API use. By having LazyMonitored implement Lazy, we can continue to pass these values directly into the ac APIs that require Lazy references. For some reason, implementing `Lazy.value` can replace `operator fun getValue` required for delegates.
…APIs.
They're currently lazy { lazy { value } }. Accessing `lazy.value`
directly allows us to make it lazy { value }. This should be more
performant and prevents us from double-counting these components.
After this, we need to add a test for counting the number of components init so we don't regress, then started working on the recursive lazy implementation. |
…t groups. By component groups, I mean I applied this to any class with the class kdoc, "Component group for...". There are a few instances of lazy we had to keep using the old API to avoid having to update constructor arguments.
…-in API use. By having LazyMonitored implement Lazy, we can continue to pass these values directly into the ac APIs that require Lazy references. For some reason, implementing `Lazy.value` can replace `operator fun getValue` required for delegates.
…APIs.
They're currently lazy { lazy { value } }. Accessing `lazy.value`
directly allows us to make it lazy { value }. This should be more
performant and prevents us from double-counting these components.
…<WebAppShortcutManager>. My goal is to convert `UseCases` to use entirely LazyComponents because it has known performance issues related to recursive lazy initialization. This will act as a proof-of-concept for this change. This is commit mozilla-mobile#1 for changing the `webAppShortcutManager` to a LazyComponent in fenix.
…nent WebAppShortcutManager. This is commit mozilla-mobile#2 of making WebAppShortcutManager a Lazy Component.
…rch... in LazyComponent. This is needed for fenix to convert searchEngineManager to LazyComponent. Unfortunately, even though I didn't add a parameter, detekt is now warning for a long parameter list. Since that's outside the scope of this PR, I suppressed it.
…<WebAppShortcutManager>. My goal is to convert `UseCases` to use entirely LazyComponents because it has known performance issues related to recursive lazy initialization. This will act as a proof-of-concept for this change. This is commit mozilla-mobile#1 for changing the `webAppShortcutManager` to a LazyComponent in fenix.
…nent WebAppShortcutManager. This is commit mozilla-mobile#2 of making WebAppShortcutManager a Lazy Component.
…rch... in LazyComponent. This is needed for fenix to convert searchEngineManager to LazyComponent. Unfortunately, even though I didn't add a parameter, detekt is now warning for a long parameter list. Since that's outside the scope of this PR, I suppressed it.
…n object. This keeps it consistent with LazyComponent and helps avoid confusion when accessing their values (i.e. ComponentInitCount can be confusing if it only represents LazyMonitored).
Something random I noticed that should be changed.
…zyComponent init count.
…rceUseTest. I ran into this issue when developing this PR and I thought it would be more intuitive in this order.
This commit series must be landed in collaboration with the changes destined for ac. This is the first commit towards turning `UseCases` into using LazyComponents.
…omponent. This is part 2 of moving UseCases to LazyComponents. Our first success! We have one less component initialized and one fewer runBlocking calls.
|
I put up two PRs as proof-of-concept using It was a lot of work to use it so I'm wondering if it's worth the effort to keep going forward. I should probably measure the difference in start up time from this PR. |
|
We're having a discussion on the best approach in the ac PR: mozilla-mobile/android-components#8909 (comment) |
…<WebAppShortcutManager>. My goal is to convert `UseCases` to use entirely LazyComponents because it has known performance issues related to recursive lazy initialization. This will act as a proof-of-concept for this change. This is commit mozilla-mobile#1 for changing the `webAppShortcutManager` to a LazyComponent in fenix.
…nent WebAppShortcutManager. This is commit mozilla-mobile#2 of making WebAppShortcutManager a Lazy Component.
…rch... in LazyComponent. This is needed for fenix to convert searchEngineManager to LazyComponent. Unfortunately, even though I didn't add a parameter, detekt is now warning for a long parameter list. Since that's outside the scope of this PR, I suppressed it.
…n object. This keeps it consistent with LazyComponent and helps avoid confusion when accessing their values (i.e. ComponentInitCount can be confusing if it only represents LazyMonitored).
…zyComponent init count.
…rceUseTest. I ran into this issue when developing this PR and I thought it would be more intuitive in this order.
This commit series must be landed in collaboration with the changes destined for ac. This is the first commit towards turning `UseCases` into using LazyComponents.
|
The If we find other components with perf issues that we can't optimize out, we can use this approach there. However, there are no actionable items to do right now without further investigation of performance issues during start up so let's close this issue. |
|
To be explicit, we made no perf optimizations in this PR but we came up with an approach to deal with under-performing components. |
MarcLeclair found a pattern that prevents kotlin's lazy initialization pattern from working as expected in nested lazy initializers when combined with our serviceLocator system:
via code sample
If
useCasesis referenced and thus initialized,search.searchEngineManagerwill unexpectedly be initialized because it's referenced to pass it into theuseCasesconstructor, even if it's not actually used in the initializer. In practice, we've identified improvements of up to 60-80ms in just one case where this occurred (inuseCases).We should try to identify a generic solution to prevent this anti-pattern for good.
Initial solution ideas
Lazy, or implement our own, to intelligently pass getters rather than the actual object┆Issue is synchronized with this Jira Task
The text was updated successfully, but these errors were encountered: