Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #4064: Replace prerelease R8 with improved Kotlin coroutines library #7153

Merged

Conversation

colintheshots
Copy link
Contributor

@colintheshots colintheshots commented Dec 13, 2019

There are no UI changes here. The only change is replacing the pre-release R8 version with a newer Kotlin coroutines library that no longer uses slow ServiceLoaders on the Main thread.

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@codecov-io
Copy link

codecov-io commented Dec 13, 2019

Codecov Report

Merging #7153 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #7153   +/-   ##
=========================================
  Coverage     18.93%   18.93%           
  Complexity      436      436           
=========================================
  Files           288      288           
  Lines         11288    11288           
  Branches       1536     1536           
=========================================
  Hits           2137     2137           
  Misses         8989     8989           
  Partials        162      162

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80693f4...e4646ca. Read the comment docs.

@mcomella mcomella self-requested a review December 13, 2019 18:50
Copy link
Contributor

@mcomella mcomella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colintheshots What is the expected performance impact for this change? As you mentioned, the new coroutines version has performance improvements and the Dispatcher will no longer be an issue (but it wasn't with the current hack). However, I'm wondering if there will be a performance impact because we're no longer using the newest version of R8 and if that impact will be positive or negative.

Note: an additional assumption of mine is that there are no functional changes.

@@ -4,9 +4,8 @@

object Versions {
const val kotlin = "1.3.30"
const val coroutines = "1.3.1"
const val coroutines = "1.3.3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No action needed. To state my understanding, in the coroutines CHANGELOG, this fix is:

Avoid ServiceLoader for loading Dispatchers.Main (#1572, #1557, #878, #1606).

sgtm.

@colintheshots
Copy link
Contributor Author

However, I'm wondering if there will be a performance impact because we're no longer using the newest version of R8 and if that impact will be positive or negative.

I believe it should be fine. The version of R8 we were using was many months old and only slightly newer than the released version. It was so close to making the release that several Google Developer Advocates believed the fix was in the release. There was one other library using a ServiceLoader. We can look for any impact on cold start tonight in the Nimbledroid reports.

@colintheshots colintheshots merged commit a8f895c into mozilla-mobile:master Dec 13, 2019
@colintheshots
Copy link
Contributor Author

profile

@hawkinsw
Copy link
Contributor

Scenario Status Time (ms)
Cold Startup success 1770
Enable private browsing success 440
Turn On Sync fail 0
Search or enter address fail 0
Add tab fail 0
More options success 360
Disable private browsing success 410
Text fail 0
Text fail 0
Enable private browsing success 400
Title success 300
Summary success 240
Title success 310
Title success 200
Summary success 210
Title success 190
Title success 310
Title success 180
Summary success 180

@colintheshots
Copy link
Contributor Author

colintheshots commented Dec 13, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants