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 decorating events with mutable device info to a background thread #214

Merged
merged 6 commits into from
May 7, 2024

Conversation

wzieba
Copy link
Member

@wzieba wzieba commented Apr 29, 2024

Description

This PR is related to improving performance of TracksClient#track method. This is crucial for performance of projects using this library - this method is called in numerous places, very often on the main thread. Internally we discussed and measure impact: paqN3M-18i-p2

Fix

I moved execution of the longest method, DeviceInformation#getMutableDeviceInfo, to a thread that is responsible for moving events to local database. As a result, getMutableDeviceInfo is executed a few milliseconds later than previously, but in reality it shouldn't affect business value that we gain from these properties.

Test

Performance testing

I've introduced benchmark module to perform these tests. I don't add CI check of any kind, as it's not trivial when it comes to performance benchmarking and requires rather a solid setup which holds results from many previous executions. (https://medium.com/androiddevelopers/fighting-regressions-with-benchmarks-in-ci-6ea9a14b5c71).

  1. Run basicTrackMethod() on a real device
  2. Write down the median value (see test result right panel > "Benchmark" tab)
  3. Revert the fix: git revert 82f903cf53b3cfcc8f8af5d81e7e1842029943af
  4. Run basicTrackMethod() on a real device again
  5. Compare results from both executions: first result should be significantly lower.

Validation testing

In this test, we want only to validate if "mutable device info" are still updated.

  1. I used WooCommerce for this. If Bump Tracks to 5.0.0 woocommerce/woocommerce-android#11416 is not yet merged, please use it as a base as it addresses breaking changes introduced with Tracks 5.0.0 release.
  2. Replace a version of Tracks with the one from this PR (recent: 214-5c57d4048541a07f1e7b8aa011366e6844e71aee)
  3. Use some network intercepting software like Charles or Proxyman
  4. Perform some navigation (e.g. open order details): see that after some time there's a call to https://public-api.wordpress.com/rest/v1.1/tracks/record with commonProps array and device_info_device_orientation in it. Verify the value was correct.
  5. Change phone orientation
  6. Perform again some operations and verify if after a few seconds there's another call to tracks, this time with opposite value for device_info_device_orientation.

Screenshots

Those are from my performance tests. They show median execution time over multiple executions.

Before

image

After

image

@wzieba
Copy link
Member Author

wzieba commented Apr 30, 2024

Because we discussed around this topic recently, and it's relatively easy to test this change on WooCommerce - @hichamboushaba do you mind reviewing this PR?

@hichamboushaba
Copy link
Member

Because we discussed around this topic recently, and it's relatively easy to test this change on WooCommerce - @hichamboushaba do you mind reviewing this PR?

Sorry for the late reply @wzieba, would it be OK to review this tomorrow or next Monday? I have to focus on finishing up a project before the code freeze.

@wzieba
Copy link
Member Author

wzieba commented May 2, 2024

No worries @hichamboushaba , tomorrow or next Monday is totally fine! Thanks

Copy link
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

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

Sorry for late review @wzieba, nice work here, a simple change that brings a big improvement 👏

@wzieba wzieba merged commit e2e7a0d into trunk May 7, 2024
10 checks passed
@wzieba wzieba deleted the performance-improvements branch May 7, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants