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

Move start call of unused services off the main thread #2127

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

jinliu9508
Copy link
Contributor

@jinliu9508 jinliu9508 commented Jun 17, 2024

Description

One Line Summary

Move start call of unused services off the main thread to make the initWithContext more efficient.

Details

Motivation

Our SDK initialization is currently taking longer to finish compare to some other SDKs. One of the reason is we construct service components like Location Manager or IAM Manager during the initialization phrase when they are not really needed until later. This PR aims to move the initialization of these services to background thread so they don't slow down the overall SDK initialization.

Scope

The retrieval of services that were changed to initialize in background is not longer from a saved instance in OneSignalImp.kt. Instead, each of these components has a getter that retrieve its instance from the service provider.

Testing

Manual testing

  • I measure how long the initialization takes by tracking the start and finish time when 'initWithContext' is called. In my testing in a pixel 3a API 34 emulator, it took an average of 600 ms to complete initialization before the change and around 300 ms after the change, which is around ~50% faster.
  • Send notification, add tags, and outcome work.
  • Calling 'initWithContext' in a background thread works. Other OneSignal calls can only work if they are called after 'initWithContext' and in the same thread.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@jinliu9508 jinliu9508 changed the title WIP: Move start call of unused services off the main thread Move start call of unused services off the main thread Jun 20, 2024
@jinliu9508 jinliu9508 requested review from nan-li and jkasten2 June 20, 2024 19:43
@jinliu9508 jinliu9508 force-pushed the get-service-by-getter branch from a5e935f to f2559c3 Compare June 21, 2024 04:36
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Left another 2 comments.

Also this existing comment is still unanswered and not addressed:
https://github.com/OneSignal/OneSignal-Android-SDK/pull/2127/files#r1644805601

@jinliu9508 jinliu9508 requested a review from jkasten2 June 25, 2024 21:49
@jinliu9508 jinliu9508 force-pushed the get-service-by-getter branch 2 times, most recently from 8f95d63 to 5469f76 Compare June 25, 2024 22:05
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Tests are failing on CI, made a comment about one test that will not pass often.

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Changes look good now, but probably want to rebase to clean up a few a few of the fixup commits.

@jinliu9508 jinliu9508 force-pushed the get-service-by-getter branch from afdfdc4 to 1346a32 Compare June 25, 2024 23:19
@jinliu9508 jinliu9508 merged commit 08c0f30 into main Jun 26, 2024
2 checks passed
@jinliu9508 jinliu9508 deleted the get-service-by-getter branch June 26, 2024 12:28
@jinliu9508 jinliu9508 mentioned this pull request Jul 2, 2024
jinliu9508 added a commit that referenced this pull request Jul 3, 2024
jinliu9508 added a commit that referenced this pull request Jul 3, 2024
Revert "Merge pull request #2127 from OneSignal/get-service-by-getter"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants