For #11753 - Update compose to 1.1.1 and Kotlin to 1.6.10 #12000

Mugurell · 2022-04-13T16:07:02Z

This also required updating room to >= 2.4.0.
This new version adds a deprecation of the MigrationTestHelper api used in
LoginExceptionStorageTest that is to be later fixed in #11765.

activity_compose was also updated to the latest stable version to ensure a
better match with the latest stable version for compose.

Most of the changes are around supporting exhaustive whens for sealed classes as a new kotlin feature - https://kotlinlang.org/docs/whatsnew1530.html#exhaustive-when-statements-for-sealed-and-boolean-subjects

Updating kotlin-coroutines will be a followup - #11755

This should help fix mozilla-mobile/fenix#23989 - stress tested the scenario that I found to reproduce that crash and it doesn't happen anymore but more tests are needed before landing the updates at once on all projects.

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

Mugurell · 2022-04-13T16:35:02Z

...esomebar/src/main/java/mozilla/components/compose/browser/awesomebar/internal/Suggestions.kt

@@ -36,30 +35,25 @@ internal fun Suggestions(
        state = state,
        modifier = Modifier.testTag("mozac.awesomebar.suggestions")
    ) {
-        val suggestions = fetcher.state.value


I think not having this fetcher that would update the data to display as a property inside of LazyColumn that would also recompose when the data changes would be the most important improvement.

I am not sure I fully understand this change :). Don't need to block on it, but curious if you could share more?

The update to a newer compose version definitely played a part but I would also put it on the fact that here we have a recomposition loop (not bad bad but bad enough - https://developer.android.com/jetpack/compose/phases#recomp-loop) and the fact that composable functions can run in parallel - https://developer.android.com/jetpack/compose/mental-model#parallel.

Not sure what synchronization mechanisms are used to prevent issue with running composables in parallel but just speculating that it may be possible that because fetcher.state.value is read in Suggestions it may happen that any change in the value will trigger:

a recomposition of the items shown (the composables which adapts those values)

this will trigger a recomposition of the parent - Suggestions

Suggestions will again read the value from fetcher.state.value. (We should've used remember{} to prevent this)

the items shouldn't recompose since the actual value didn't change but maybe it did in the meantime.

Maybe these updates happen in less than a frame and recompositions didn't have time to handle it all, maybe the states are updated and a new composition is driven by different cores which may have out of sync values.
There are many "maybe"s but by moving that code for observing state out of the composable that should display the state the code becomes cleaner, more in line with Compose's direction of passing state down to be displayed - https://developer.android.com/jetpack/compose/state and hopefully skips some recompositions or at least leads a more direct line of updates, without that loop I talked about above.

csadilek

Just putting the do not land label on re:

This should help fix mozilla-mobile/fenix#23989 - stress tested the scenario that I found to reproduce that crash and it doesn't happen anymore but more tests are needed before landing the updates at once on all projects.

@Mugurell When you write "more tests needed" did you mean prior to landing this or are you confident already? In either case we should land this on Monday or early in the week in case those crashes do come back.

MozillaNoah · 2022-04-13T22:39:24Z

...e/logins/src/androidTest/java/mozilla/components/feature/logins/LoginExceptionStorageTest.kt

@@ -37,6 +37,7 @@ class LoginExceptionStorageTest {
    var instantTaskExecutorRule = InstantTaskExecutorRule()

    @get:Rule
+    @Suppress("DEPRECATION")


what's throwing the "deprecation" warning here?

Left a note in the commit message. The constructor used there will become deprecated following the room version update. To be handled in #11765.

Mugurell · 2022-04-14T06:24:24Z

Just putting the do not land label on re:

This should help fix mozilla-mobile/fenix#23989 - stress tested the scenario that I found to reproduce that crash and it doesn't happen anymore but more tests are needed before landing the updates at once on all projects.

@Mugurell When you write "more tests needed" did you mean prior to landing this or are you confident already? In either case we should land this on Monday or early in the week in case those crashes do come back.

I didn't find a clear cause or clear steps to reproduce that crash but found a general scenario that would trigger it "most of the time" in under 5 minutes (usually <1/2/3 minutes).
With this speculative fix it didn't crash anymore though I tried the same scenario for 30 minutes.
These tests were done with the repos at the state when the crash happened.
Will rebase everything and test again with my fingers crossed.
Then I agree that we should try to land at the beginning of the next week to have a few days for observation and be able to quickly revert if needed.

This also required updating room to >= 2.4.0. This new version adds a deprecation of the `MigrationTestHelper` api used in `LoginExceptionStorageTest` that is to be later fixed in mozilla-mobile#11765. `activity_compose` was also update to the latest stable version to ensure a better match with the latest stable version for compose. Used 1.6.10 for Kotlin although 1.6.20 is available to prevent any issues with Compose 1.1.1 reported as an error at compile time: "e: This version (1.1.1) of the Compose Compiler requires Kotlin version 1.6.10 but you appear to be using Kotlin version 1.6.20 which is not known to be compatible. Please fix your configuration (or `suppressKotlinVersionCompatibilityCheck` but don't say I didn't warn you!)."

Speculative fix for mozilla-mobile/fenix#23989.

Mugurell · 2022-04-14T14:04:17Z

Tested again with this applied on latest main and it seems like the crash dissapeared. 🤞

gabrielluong

Compose changes look good to me. I did review the entire PR to be clear.

Mugurell · 2022-04-18T16:16:42Z

Confirmed with Christian that we can land this and keep a close eye on Sentry.

Mugurell requested review from Amejia481, gabrielluong, grigoryk, jonalmeida, a team and csadilek as code owners April 13, 2022 16:07

Mugurell commented Apr 13, 2022

View reviewed changes

csadilek added the do not land PRs that requires coordination before landing label Apr 13, 2022

csadilek reviewed Apr 13, 2022

View reviewed changes

MozillaNoah reviewed Apr 13, 2022

View reviewed changes

Mugurell added 2 commits April 14, 2022 11:58

For mozilla-mobile#11753 - Optimize AwesomeBar recompositions

806f974

Speculative fix for mozilla-mobile/fenix#23989.

Mugurell mentioned this pull request Apr 14, 2022

For #11753 - Update compose to 1.1.1 and Kotlin to 1.6.10 mozilla-mobile/fenix#24796

Merged

3 tasks

Mugurell force-pushed the updateCompose2 branch from 70a8dfe to 806f974 Compare April 14, 2022 14:02

Mugurell mentioned this pull request Apr 14, 2022

WIP Update to Kotlin 1.6 #11516

Closed

4 tasks

gabrielluong approved these changes Apr 14, 2022

View reviewed changes

Mugurell changed the title ~~For #11753 - Update compose to 1.1.1 and Kotlin to 1.6.20~~ For #11753 - Update compose to 1.1.1 and Kotlin to 1.6.10 Apr 14, 2022

Mugurell mentioned this pull request Apr 14, 2022

For #11755 - Update coroutines to 1.6.1. Update tests. #12006

Merged

4 tasks

Mugurell added 🛬 needs landing PRs that are ready to land and removed do not land PRs that requires coordination before landing labels Apr 18, 2022

Merge branch 'main' into updateCompose2

156222e

mergify bot merged commit 62fe384 into mozilla-mobile:main Apr 18, 2022

This was referenced Apr 20, 2022

Intermittent UI test failure - < SettingsSearchTest. toggleSearchSuggestionsTest > mozilla-mobile/fenix#23817

Closed

Search suggestions don't show up on cold run mozilla-mobile/fenix#23864

Closed

chutten mentioned this pull request May 11, 2022

Bug 1746180 - Implement Rate, Denominator and Numerator using UniFFI mozilla/glean#2048

Merged

Mugurell deleted the updateCompose2 branch August 5, 2022 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #11753 - Update compose to 1.1.1 and Kotlin to 1.6.10 #12000

For #11753 - Update compose to 1.1.1 and Kotlin to 1.6.10 #12000

Mugurell commented Apr 13, 2022

Mugurell Apr 13, 2022

csadilek Apr 14, 2022

Mugurell Apr 15, 2022 •

edited

Loading

csadilek left a comment

MozillaNoah Apr 13, 2022

Mugurell Apr 14, 2022

Mugurell commented Apr 14, 2022

Mugurell commented Apr 14, 2022

gabrielluong left a comment

Mugurell commented Apr 18, 2022

For #11753 - Update compose to 1.1.1 and Kotlin to 1.6.10 #12000

For #11753 - Update compose to 1.1.1 and Kotlin to 1.6.10 #12000

Conversation

Mugurell commented Apr 13, 2022

Pull Request checklist

After merge

Mugurell Apr 13, 2022

Choose a reason for hiding this comment

csadilek Apr 14, 2022

Choose a reason for hiding this comment

Mugurell Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

csadilek left a comment

Choose a reason for hiding this comment

MozillaNoah Apr 13, 2022

Choose a reason for hiding this comment

Mugurell Apr 14, 2022

Choose a reason for hiding this comment

Mugurell commented Apr 14, 2022

Mugurell commented Apr 14, 2022

gabrielluong left a comment

Choose a reason for hiding this comment

Mugurell commented Apr 18, 2022

Mugurell Apr 15, 2022 •

edited

Loading