-
-
Notifications
You must be signed in to change notification settings - Fork 449
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 to a single NetworkCallback listener to reduce number of IPC calls #4164
Move to a single NetworkCallback listener to reduce number of IPC calls #4164
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c2c78de | 415.28 ms | 505.08 ms | 89.80 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c2c78de | 1.58 MiB | 2.21 MiB | 640.27 KiB |
Previous results on branch: markushi/enh/reduce-number-of-ipc-calls-connection-status-provider
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f6eabd8 | 471.24 ms | 526.07 ms | 54.83 ms |
feeddf1 | 424.94 ms | 463.50 ms | 38.56 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f6eabd8 | 1.58 MiB | 2.21 MiB | 641.13 KiB |
feeddf1 | 1.58 MiB | 2.21 MiB | 641.12 KiB |
@@ -41,7 +44,7 @@ public AndroidConnectionStatusProvider( | |||
this.context = ContextUtils.getApplicationContext(context); | |||
this.logger = logger; | |||
this.buildInfoProvider = buildInfoProvider; | |||
this.registeredCallbacks = new HashMap<>(); | |||
this.connectionStatusObservers = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just thinking if it's better to use CopyOnWrite here to avoid potential lock contention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine, I'm using a lock mechanism for safe access and there aren't many writes to this list: In my testing there were 3 listeners attached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, very simple change with a very big impact 🚀
Oh btw, I also thought of maybe persisting the last known connection status in |
…ection-status-provider
📜 Description
We don't seem to be able to get rid of calling
getConnectionStatus()
on connectivity change, as the NetworkCallback seems to fire for any network, e.g. reporting a disconnect of WIFI, although Cellular connection is still available.But I noticed every internal
IConnectionStatusObserver
was tied to it's ownNetworkCallback
, causing us to callgetConnectionStatus()
per change per observer - which quickly blows our number of IPC calls up.So instead let's have a single
NetworkCallback
, callgetConnectionStatus()
once per network change and then broadcast the state to all observers. This should already reduce the number of IPC calls by 3.💡 Motivation and Context
Less IPC calls
Fixes #4053
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps