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

Implement Android Executors for Storage #4830

Merged
merged 26 commits into from Apr 26, 2023

Conversation

maneesht
Copy link
Contributor

@maneesht maneesht commented Mar 28, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Mar 28, 2023

📝 PRs merging into main branch

Our main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released.

Add the 'main-merge-ack' label to your PR to confirm merging into the main branch is intended.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 28, 2023

Coverage Report 1

Affected Products

  • firebase-storage

    Overall coverage changed from 86.21% (f47a0dd) to 86.16% (554ec41) by -0.05%.

    FilenameBase (f47a0dd)Merge (554ec41)Diff
    SmartHandler.java87.50%92.31%+4.81%
    StorageTaskScheduler.java100.00%95.45%-4.55%
    UploadTask.java83.11%83.17%+0.06%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/7hENhLIisc.html

@github-actions
Copy link
Contributor

github-actions bot commented Mar 28, 2023

Unit Test Results

  36 files  ±0    36 suites  ±0   6m 21s ⏱️ ±0s
126 tests ±0  125 ✔️ +1  0 💤 ±0  1  - 1 
252 runs  ±0  251 ✔️ +1  0 💤 ±0  1  - 1 

For more details on these failures, see this check.

Results for commit 024171d. ± Comparison against base commit 4c833f6.

♻️ This comment has been updated with latest results.

@maneesht maneesht requested a review from rlazo March 28, 2023 23:18
@maneesht maneesht marked this pull request as ready for review March 28, 2023 23:18
@maneesht
Copy link
Contributor Author

@rlazo - It seems like creating a LimitedConcurrencyExecutor with only 1 as the concurrency causes many of the tests to fail, as for some reason a different thread calls the callback than the original one created. I fixed this by using a SequentialExecutor

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 28, 2023

Size Report 1

Affected Products

  • firebase-storage

    TypeBase (f47a0dd)Merge (554ec41)Diff
    aar116 kB115 kB-840 B (-0.7%)
    apk (aggressive)359 kB361 kB+1.39 kB (+0.4%)
    apk (release)1.59 MB1.59 MB+2.47 kB (+0.2%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/SBYRW6gePx.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 29, 2023

Startup Time Report 1

Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS.

Notes

Startup Times

  • fire-app-check

    DeviceStatisticsDistributions
    oriole-32
    Percentilef47a0dd554ec41DiffSignificant (?)
    p10452 ±699 μs315 ±232 μs-138 μs (-30.5%)NO
    p25475 ±717 μs331 ±242 μs-144 μs (-30.3%)NO
    p50520 ±770 μs360 ±261 μs-159 μs (-30.7%)NO
    p75621 ±918 μs431 ±310 μs-190 μs (-30.5%)NO
    p90778 ±1035 μs542 ±379 μs-237 μs (-30.4%)NO

    20 test runs in comparison
    CommitTest Runs
    f47a0dd
    • 2023-04-26_17:14:58.819035_Juch
    • 2023-04-26_17:14:58.821633_kicn
    • 2023-04-26_17:14:58.821644_BIyy
    • 2023-04-26_17:14:58.821655_ZzWI
    • 2023-04-26_17:14:58.821661_wfov
    • 2023-04-26_17:14:58.821666_Tmac
    • 2023-04-26_17:14:58.821672_hwqT
    • 2023-04-26_17:14:58.821677_bbpd
    • 2023-04-26_17:14:58.821683_iOFQ
    • 2023-04-26_17:14:58.821689_oVVp
    554ec41
    • 2023-04-26_18:28:27.355295_TNSQ
    • 2023-04-26_18:28:27.358167_FPFw
    • 2023-04-26_18:28:27.358390_MhRy
    • 2023-04-26_18:28:27.358400_zhwk
    • 2023-04-26_18:28:27.358406_zpMj
    • 2023-04-26_18:28:27.358412_juop
    • 2023-04-26_18:28:27.358418_Ksqp
    • 2023-04-26_18:28:27.358425_VQvM
    • 2023-04-26_18:28:27.358431_zgpP
    • 2023-04-26_18:28:27.358437_xjZY
    redfin-30
    Percentilef47a0dd554ec41DiffSignificant (?)
    p101.75 ±2 ms421 ±92 μs-1.33 ms (-76.0%)NO
    p251.89 ±2 ms465 ±164 μs-1.42 ms (-75.4%)NO
    p502.13 ±3 ms527 ±252 μs-1.60 ms (-75.2%)NO
    p752.45 ±3 ms663 ±437 μs-1.78 ms (-72.9%)NO
    p903.00 ±4 ms919 ±605 μs-2.08 ms (-69.3%)NO

    20 test runs in comparison
    CommitTest Runs
    f47a0dd
    • 2023-04-26_17:14:58.819035_Juch
    • 2023-04-26_17:14:58.821633_kicn
    • 2023-04-26_17:14:58.821644_BIyy
    • 2023-04-26_17:14:58.821655_ZzWI
    • 2023-04-26_17:14:58.821661_wfov
    • 2023-04-26_17:14:58.821666_Tmac
    • 2023-04-26_17:14:58.821672_hwqT
    • 2023-04-26_17:14:58.821677_bbpd
    • 2023-04-26_17:14:58.821683_iOFQ
    • 2023-04-26_17:14:58.821689_oVVp
    554ec41
    • 2023-04-26_18:28:27.355295_TNSQ
    • 2023-04-26_18:28:27.358167_FPFw
    • 2023-04-26_18:28:27.358390_MhRy
    • 2023-04-26_18:28:27.358400_zhwk
    • 2023-04-26_18:28:27.358406_zpMj
    • 2023-04-26_18:28:27.358412_juop
    • 2023-04-26_18:28:27.358418_Ksqp
    • 2023-04-26_18:28:27.358425_VQvM
    • 2023-04-26_18:28:27.358431_zgpP
    • 2023-04-26_18:28:27.358437_xjZY
  • fire-gcs

    DeviceStatisticsDistributions
    oriole-32
    Percentilef47a0dd554ec41DiffSignificant (?)
    p1012.9 ±3 μs41.0 ±2 μs+28.1 μs (+217.7%)YES
    p2513.5 ±3 μs43.8 ±3 μs+30.3 μs (+225.0%)YES
    p5014.6 ±3 μs48.2 ±6 μs+33.5 μs (+229.1%)YES
    p7517.5 ±4 μs55.6 ±10 μs+38.1 μs (+217.1%)YES
    p9019.6 ±5 μs70.4 ±22 μs+50.8 μs (+259.2%)YES

    20 test runs in comparison
    CommitTest Runs
    f47a0dd
    • 2023-04-26_17:14:58.819035_Juch
    • 2023-04-26_17:14:58.821633_kicn
    • 2023-04-26_17:14:58.821644_BIyy
    • 2023-04-26_17:14:58.821655_ZzWI
    • 2023-04-26_17:14:58.821661_wfov
    • 2023-04-26_17:14:58.821666_Tmac
    • 2023-04-26_17:14:58.821672_hwqT
    • 2023-04-26_17:14:58.821677_bbpd
    • 2023-04-26_17:14:58.821683_iOFQ
    • 2023-04-26_17:14:58.821689_oVVp
    554ec41
    • 2023-04-26_18:28:27.355295_TNSQ
    • 2023-04-26_18:28:27.358167_FPFw
    • 2023-04-26_18:28:27.358390_MhRy
    • 2023-04-26_18:28:27.358400_zhwk
    • 2023-04-26_18:28:27.358406_zpMj
    • 2023-04-26_18:28:27.358412_juop
    • 2023-04-26_18:28:27.358418_Ksqp
    • 2023-04-26_18:28:27.358425_VQvM
    • 2023-04-26_18:28:27.358431_zgpP
    • 2023-04-26_18:28:27.358437_xjZY
    redfin-30
    Percentilef47a0dd554ec41DiffSignificant (?)
    p1027.8 ±8 μs88.8 ±13 μs+61.0 μs (+219.7%)YES
    p2529.0 ±9 μs92.6 ±14 μs+63.5 μs (+218.8%)YES
    p5030.9 ±9 μs99.6 ±17 μs+68.7 μs (+222.7%)YES
    p7535.6 ±12 μs110 ±24 μs+74.9 μs (+210.5%)YES
    p9041.6 ±19 μs130 ±41 μs+88.5 μs (+212.6%)MAYBE

    20 test runs in comparison
    CommitTest Runs
    f47a0dd
    • 2023-04-26_17:14:58.819035_Juch
    • 2023-04-26_17:14:58.821633_kicn
    • 2023-04-26_17:14:58.821644_BIyy
    • 2023-04-26_17:14:58.821655_ZzWI
    • 2023-04-26_17:14:58.821661_wfov
    • 2023-04-26_17:14:58.821666_Tmac
    • 2023-04-26_17:14:58.821672_hwqT
    • 2023-04-26_17:14:58.821677_bbpd
    • 2023-04-26_17:14:58.821683_iOFQ
    • 2023-04-26_17:14:58.821689_oVVp
    554ec41
    • 2023-04-26_18:28:27.355295_TNSQ
    • 2023-04-26_18:28:27.358167_FPFw
    • 2023-04-26_18:28:27.358390_MhRy
    • 2023-04-26_18:28:27.358400_zhwk
    • 2023-04-26_18:28:27.358406_zpMj
    • 2023-04-26_18:28:27.358412_juop
    • 2023-04-26_18:28:27.358418_Ksqp
    • 2023-04-26_18:28:27.358425_VQvM
    • 2023-04-26_18:28:27.358431_zgpP
    • 2023-04-26_18:28:27.358437_xjZY
  • timeToInitialDisplay

    DeviceStatisticsDistributions
    oriole-32
    Percentilef47a0dd554ec41DiffSignificant (?)
    p10193 ±8 ms199 ±7 ms+6.59 ms (+3.4%)NO
    p25199 ±9.6 ms206 ±7 ms+6.60 ms (+3.3%)NO
    p50207 ±14 ms214 ±7 ms+6.19 ms (+3.0%)NO
    p75218 ±21 ms224 ±7 ms+5.47 ms (+2.5%)NO
    p90228 ±26 ms236 ±9 ms+7.76 ms (+3.4%)NO

    20 test runs in comparison
    CommitTest Runs
    f47a0dd
    • 2023-04-26_17:14:58.819035_Juch
    • 2023-04-26_17:14:58.821633_kicn
    • 2023-04-26_17:14:58.821644_BIyy
    • 2023-04-26_17:14:58.821655_ZzWI
    • 2023-04-26_17:14:58.821661_wfov
    • 2023-04-26_17:14:58.821666_Tmac
    • 2023-04-26_17:14:58.821672_hwqT
    • 2023-04-26_17:14:58.821677_bbpd
    • 2023-04-26_17:14:58.821683_iOFQ
    • 2023-04-26_17:14:58.821689_oVVp
    554ec41
    • 2023-04-26_18:28:27.355295_TNSQ
    • 2023-04-26_18:28:27.358167_FPFw
    • 2023-04-26_18:28:27.358390_MhRy
    • 2023-04-26_18:28:27.358400_zhwk
    • 2023-04-26_18:28:27.358406_zpMj
    • 2023-04-26_18:28:27.358412_juop
    • 2023-04-26_18:28:27.358418_Ksqp
    • 2023-04-26_18:28:27.358425_VQvM
    • 2023-04-26_18:28:27.358431_zgpP
    • 2023-04-26_18:28:27.358437_xjZY
    redfin-30
    Percentilef47a0dd554ec41DiffSignificant (?)
    p10230 ±4 ms254 ±3 ms+24.5 ms (+10.7%)YES
    p25236 ±4 ms261 ±2 ms+25.4 ms (+10.8%)YES
    p50243 ±4 ms268 ±3 ms+25.8 ms (+10.6%)YES
    p75251 ±5 ms277 ±3 ms+25.9 ms (+10.3%)YES
    p90260 ±6 ms290 ±7 ms+30.1 ms (+11.6%)MAYBE

    20 test runs in comparison
    CommitTest Runs
    f47a0dd
    • 2023-04-26_17:14:58.819035_Juch
    • 2023-04-26_17:14:58.821633_kicn
    • 2023-04-26_17:14:58.821644_BIyy
    • 2023-04-26_17:14:58.821655_ZzWI
    • 2023-04-26_17:14:58.821661_wfov
    • 2023-04-26_17:14:58.821666_Tmac
    • 2023-04-26_17:14:58.821672_hwqT
    • 2023-04-26_17:14:58.821677_bbpd
    • 2023-04-26_17:14:58.821683_iOFQ
    • 2023-04-26_17:14:58.821689_oVVp
    554ec41
    • 2023-04-26_18:28:27.355295_TNSQ
    • 2023-04-26_18:28:27.358167_FPFw
    • 2023-04-26_18:28:27.358390_MhRy
    • 2023-04-26_18:28:27.358400_zhwk
    • 2023-04-26_18:28:27.358406_zpMj
    • 2023-04-26_18:28:27.358412_juop
    • 2023-04-26_18:28:27.358418_Ksqp
    • 2023-04-26_18:28:27.358425_VQvM
    • 2023-04-26_18:28:27.358431_zgpP
    • 2023-04-26_18:28:27.358437_xjZY

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/aZnFWZ1Szr/index.html

@rlazo
Copy link
Collaborator

rlazo commented Mar 29, 2023

@rlazo - It seems like creating a LimitedConcurrencyExecutor with only 1 as the concurrency causes many of the tests to fail, as for some reason a different thread calls the callback than the original one created. I fixed this by using a SequentialExecutor

I didn't see any changes to the tests , where did you made the change?

@maneesht
Copy link
Contributor Author

@rlazo - It seems like creating a LimitedConcurrencyExecutor with only 1 as the concurrency causes many of the tests to fail, as for some reason a different thread calls the callback than the original one created. I fixed this by using a SequentialExecutor

I didn't see any changes to the tests , where did you made the change?

I was referring to the change made here: https://github.com/firebase/firebase-android-sdk/pull/4830/files#diff-8a3cd8845e80a432177d61c12b761f03d6da86be0b22b62ee0de1d5341a8fbefR44

@rlazo
Copy link
Collaborator

rlazo commented Apr 5, 2023

If you sync past #4854 tests should start passing. Thanks!

@maneesht maneesht force-pushed the mtewani/storage-android-executor branch from 948db98 to 7ed6c14 Compare April 18, 2023 21:29
@maneesht maneesht enabled auto-merge (squash) April 26, 2023 00:18
@maneesht maneesht merged commit 55e77f0 into master Apr 26, 2023
18 of 20 checks passed
@maneesht maneesht deleted the mtewani/storage-android-executor branch April 26, 2023 19:03
VinayGuthal added a commit that referenced this pull request Apr 26, 2023
VinayGuthal added a commit that referenced this pull request Apr 26, 2023
VinayGuthal added a commit that referenced this pull request Apr 26, 2023
VinayGuthal added a commit that referenced this pull request Apr 26, 2023
VinayGuthal added a commit that referenced this pull request Apr 26, 2023
* Implement Android Executors for Storage (#4830)

* Revert "Refactor PublishingPlugin (#4923)" (#4937)

This reverts commit 0efc6c7.

* Revert "Refactor PublishingPlugin (#4923)"

This reverts commit 0efc6c7.

---------

Co-authored-by: Maneesh Tewani <maneesht@users.noreply.github.com>
@firebase firebase locked and limited conversation to collaborators Jun 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants