Implement Android Executors for Storage by maneesht · Pull Request #4830 · firebase/firebase-android-sdk

maneesht · 2023-03-28T23:01:19Z

github-actions · 2023-03-28T23:01:32Z

📝 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.

…roid-executor

google-oss-bot · 2023-03-28T23:10:23Z

Coverage Report ¹

Affected Products

firebase-storage
Overall coverage changed from 86.21% (f47a0dd) to 86.16% (554ec41) by -0.05%.
Filename Base (f47a0dd) Merge (554ec41) Diff
SmartHandler.java 87.50% 92.31% +4.81%
StorageTaskScheduler.java 100.00% 95.45% -4.55%
UploadTask.java 83.11% 83.17% +0.06%

Test Logs

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

github-actions · 2023-03-28T23:13:53Z

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 · 2023-03-28T23:20:01Z

@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 · 2023-03-28T23:45:04Z

Size Report ¹

Affected Products

firebase-storage
Type Base (f47a0dd) Merge (554ec41) Diff
aar 116 kB 115 kB -840 B (-0.7%)
apk (aggressive) 359 kB 361 kB +1.39 kB (+0.4%)
apk (release) 1.59 MB 1.59 MB +2.47 kB (+0.2%)

Test Logs

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

google-oss-bot · 2023-03-29T00:04:08Z

Startup Time Report ¹

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

Notes

This report is for comparing the base commit (f47a0dd) and the CI merge commit (554ec41)
Please check below reports for each individual commit to find more details (Perfetto traces, histograms, detailed measurements)
- f47a0dd: https://storage.googleapis.com/firebase-sdk-metric-reports/4IwAQLnPX6/index.html
- 554ec41: https://storage.googleapis.com/firebase-sdk-metric-reports/t9kHfpwH2a/index.html

Startup Times

`fire-app-check`

Device Statistics Distributions

oriole-32

Percentile	`f47a0dd`	`554ec41`	Diff	Significant (?)
p10	452 ±699 μs	315 ±232 μs	-138 μs (-30.5%)	NO
p25	475 ±717 μs	331 ±242 μs	-144 μs (-30.3%)	NO
p50	520 ±770 μs	360 ±261 μs	-159 μs (-30.7%)	NO
p75	621 ±918 μs	431 ±310 μs	-190 μs (-30.5%)	NO
p90	778 ±1035 μs	542 ±379 μs	-237 μs (-30.4%)	NO

20 test runs in comparison

Commit	Test 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

Percentile	`f47a0dd`	`554ec41`	Diff	Significant (?)
p10	1.75 ±2 ms	421 ±92 μs	-1.33 ms (-76.0%)	NO
p25	1.89 ±2 ms	465 ±164 μs	-1.42 ms (-75.4%)	NO
p50	2.13 ±3 ms	527 ±252 μs	-1.60 ms (-75.2%)	NO
p75	2.45 ±3 ms	663 ±437 μs	-1.78 ms (-72.9%)	NO
p90	3.00 ±4 ms	919 ±605 μs	-2.08 ms (-69.3%)	NO

20 test runs in comparison

Commit	Test 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`

Device Statistics Distributions

oriole-32

Percentile	`f47a0dd`	`554ec41`	Diff	Significant (?)
p10	12.9 ±3 μs	41.0 ±2 μs	+28.1 μs (+217.7%)	YES
p25	13.5 ±3 μs	43.8 ±3 μs	+30.3 μs (+225.0%)	YES
p50	14.6 ±3 μs	48.2 ±6 μs	+33.5 μs (+229.1%)	YES
p75	17.5 ±4 μs	55.6 ±10 μs	+38.1 μs (+217.1%)	YES
p90	19.6 ±5 μs	70.4 ±22 μs	+50.8 μs (+259.2%)	YES

20 test runs in comparison

Commit	Test 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

Percentile	`f47a0dd`	`554ec41`	Diff	Significant (?)
p10	27.8 ±8 μs	88.8 ±13 μs	+61.0 μs (+219.7%)	YES
p25	29.0 ±9 μs	92.6 ±14 μs	+63.5 μs (+218.8%)	YES
p50	30.9 ±9 μs	99.6 ±17 μs	+68.7 μs (+222.7%)	YES
p75	35.6 ±12 μs	110 ±24 μs	+74.9 μs (+210.5%)	YES
p90	41.6 ±19 μs	130 ±41 μs	+88.5 μs (+212.6%)	MAYBE

20 test runs in comparison

Commit	Test 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`

Device Statistics Distributions

oriole-32

Percentile	`f47a0dd`	`554ec41`	Diff	Significant (?)
p10	193 ±8 ms	199 ±7 ms	+6.59 ms (+3.4%)	NO
p25	199 ±9.6 ms	206 ±7 ms	+6.60 ms (+3.3%)	NO
p50	207 ±14 ms	214 ±7 ms	+6.19 ms (+3.0%)	NO
p75	218 ±21 ms	224 ±7 ms	+5.47 ms (+2.5%)	NO
p90	228 ±26 ms	236 ±9 ms	+7.76 ms (+3.4%)	NO

20 test runs in comparison

Commit	Test 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

Percentile	`f47a0dd`	`554ec41`	Diff	Significant (?)
p10	230 ±4 ms	254 ±3 ms	+24.5 ms (+10.7%)	YES
p25	236 ±4 ms	261 ±2 ms	+25.4 ms (+10.8%)	YES
p50	243 ±4 ms	268 ±3 ms	+25.8 ms (+10.6%)	YES
p75	251 ±5 ms	277 ±3 ms	+25.9 ms (+10.3%)	YES
p90	260 ±6 ms	290 ±7 ms	+30.1 ms (+11.6%)	MAYBE

20 test runs in comparison

Commit	Test 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

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

rlazo · 2023-03-29T16:31:30Z

@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 · 2023-03-29T16:53:34Z

@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 · 2023-04-05T17:13:40Z

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

…roid-executor

This reverts commit 55e77f0.

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

maneesht added 2 commits March 27, 2023 14:53

Created four separate pools

ff7b3f5

Used correct executor

e01aec4

google-oss-bot added the do-not-merge/work-in-progress label Mar 28, 2023

google-oss-bot added the size/L label Mar 28, 2023

Merge remote-tracking branch 'origin/master' into mtewani/storage-and…

22a165a

…roid-executor

maneesht added 2 commits March 28, 2023 16:16

Removed comment and moved pool sizes to static varsZ

694a095

Reverted controllableschedulerhelper

ad95939

maneesht added the main-merge-ack label Mar 28, 2023

maneesht requested a review from rlazo March 28, 2023 23:18

maneesht marked this pull request as ready for review March 28, 2023 23:18

google-oss-bot removed the do-not-merge/work-in-progress label Mar 28, 2023

maneesht requested a review from tonyjhuang March 28, 2023 23:20

Reran linter

e085d58

Undid firebase-common change

d43b907

rlazo reviewed Mar 29, 2023

View reviewed changes

Comment thread firebase-storage/src/main/java/com/google/firebase/storage/StorageRegistrar.java Outdated

Comment thread firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorageComponent.java Outdated

Addressed comments

7f58222

rlazo approved these changes Mar 29, 2023

View reviewed changes

WIP

30c2f2c

maneesht added 3 commits April 7, 2023 11:10

Merge remote-tracking branch 'origin/master' into mtewani/storage-and…

89b1303

…roid-executor

Updated tests to include uithread

2bf0754

WIP

a2de9b5

maneesht added 2 commits April 17, 2023 09:26

Fixed formatting

336dc6c

Merge remote-tracking branch 'origin/master' into mtewani/storage-and…

7ed6c14

…roid-executor

maneesht force-pushed the mtewani/storage-android-executor branch from 948db98 to 7ed6c14 Compare April 18, 2023 21:29

Updated retry count

b9a2d4d

rlazo reviewed Apr 24, 2023

View reviewed changes

Comment thread firebase-storage/src/test/java/com/google/firebase/storage/TestUtil.java Outdated

Comment thread firebase-storage/src/testUtil/java/com/google/firebase/storage/TestUploadHelper.java Outdated

maneesht added 2 commits April 24, 2023 08:52

Removed unnecessary changes

b8e467b

Replaced test statement

ff4912d

tonyjhuang approved these changes Apr 24, 2023

View reviewed changes

maneesht and others added 7 commits April 24, 2023 09:22

Merge branch 'master' into mtewani/storage-android-executor

7e37bf4

Added comment and changed throw to a warn

590056a

Attached buganizer link

13f47a8

Debug lines added

8a836f1

Fixed formatting

cf6e553

Debug prints

0c5062b

Removed print

16535b0

maneesht enabled auto-merge (squash) April 26, 2023 00:18

maneesht and others added 2 commits April 25, 2023 17:23

Merge branch 'master' into mtewani/storage-android-executor

4ca73c8

Removed unnecessary test

024171d

maneesht merged commit 55e77f0 into master Apr 26, 2023

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

Revert "Implement Android Executors for Storage (#4830)"

e5c309d

This reverts commit 55e77f0.

VinayGuthal added a commit that referenced this pull request Apr 26, 2023

Revert "Implement Android Executors for Storage (#4830)" (#4938)

1eae91b

This reverts commit 55e77f0.

VinayGuthal added a commit that referenced this pull request Apr 26, 2023

Revert "Implement Android Executors for Storage (#4830)"

fd6d768

This reverts commit 55e77f0.

VinayGuthal added a commit that referenced this pull request Apr 26, 2023

Revert "Implement Android Executors for Storage (#4830)" (#4942)

9753a66

This reverts commit 55e77f0.

firebase locked and limited conversation to collaborators Jun 1, 2023

Conversation

maneesht commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 PRs merging into main branch

Uh oh!

google-oss-bot commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-storage

Test Logs

Uh oh!

github-actions Bot commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

maneesht commented Mar 28, 2023

Uh oh!

google-oss-bot commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

firebase-storage

Test Logs

Uh oh!

google-oss-bot commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Startup Time Report 1

Notes

Startup Times

fire-app-check

fire-gcs

timeToInitialDisplay

Uh oh!

Uh oh!

Uh oh!

rlazo commented Mar 29, 2023

Uh oh!

maneesht commented Mar 29, 2023

Uh oh!

rlazo commented Apr 5, 2023

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maneesht commented Mar 28, 2023 •

edited

Loading

github-actions Bot commented Mar 28, 2023 •

edited

Loading

google-oss-bot commented Mar 28, 2023 •

edited

Loading

Coverage Report ¹

`firebase-storage`

github-actions Bot commented Mar 28, 2023 •

edited

Loading

google-oss-bot commented Mar 28, 2023 •

edited

Loading

Size Report ¹

`firebase-storage`

google-oss-bot commented Mar 29, 2023 •

edited

Loading

Startup Time Report ¹

`fire-app-check`

`fire-gcs`

`timeToInitialDisplay`