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

Optimize implementation of internal state change observers #1274

Merged
merged 5 commits into from
Jun 18, 2021

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Jun 16, 2021

Goal

Optimizes the implementation of the internal system used for notifying observers of state changes.

Changeset

This new solution avoids the performance disadvantages of the previous solution:

  • The StateEvent message is lazily created which avoids object instantiation if no observers are registered
  • BaseObservable avoids creating a Vector by replacing update() with onStateChange() (this also grants additional type safety)
  • BaseObservable uses CopyOnWriteArrayList which avoids the need for synchronization when adding/removing observers. This was seen as acceptable as observers will be added/removed very rarely, whereas the collection will be iterated over many times

The remaining changes entail updating the rest of the code and the tests to use the new method signatures.

Testing

Updated existing unit test coverage and relied on existing tests.

Performance

Profiled app to confirm there was no performance regression, and also checked CI stats.

changes_flame

baseline_flame

@fractalwrench fractalwrench changed the title Optimize implementation of internal state change observables Optimize implementation of internal state change observers Jun 16, 2021
@fractalwrench fractalwrench marked this pull request as ready for review June 17, 2021 09:30
@bugsnagbot
Copy link
Collaborator

Android notifier sizes

Format Size impact of Bugsnag (kB) Size impact of Bugsnag when Minified (kB)
APK 1513.92 1409.88
arm64_v8a 406.2 299.71
armeabi 385.72 279.22
armeabi_v7a 369.34 262.85
x86 443.05 336.55
x86_64 426.67 320.18

Generated by 🚫 Danger

Copy link
Contributor

@lemnik lemnik left a comment

Choose a reason for hiding this comment

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

LGTM

@fractalwrench fractalwrench merged commit 047e225 into next Jun 18, 2021
@fractalwrench fractalwrench deleted the PLAT-6747/observer-refactor branch June 18, 2021 08:14
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.

3 participants