Skip to content

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

Merged
jinliu9508 merged 1 commit into
mainfrom
get-service-by-getter
Jun 26, 2024
Merged

Move start call of unused services off the main thread#2127
jinliu9508 merged 1 commit into
mainfrom
get-service-by-getter

Conversation

@jinliu9508

@jinliu9508 jinliu9508 commented Jun 17, 2024

Copy link
Copy Markdown
Contributor

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

Comment thread OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt Outdated
Comment thread OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt Outdated
Comment thread OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt Outdated
@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 jkasten2 and nan-li 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

@jkasten2 jkasten2 left a comment

Copy link
Copy Markdown
Member

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

@jkasten2 jkasten2 left a comment

Copy link
Copy Markdown
Member

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.

@jkasten2 jkasten2 left a comment

Copy link
Copy Markdown
Member

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
@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