Migrate some executors in fad/in-app-feedback #4350

lfkellogg · 2022-11-21T19:19:52Z

No description provided.

google-oss-bot · 2022-11-21T19:20:22Z

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add the 'no-changelog' label to the PR to silence this warning.)

Generated by 🚫 Danger

google-oss-bot · 2022-11-21T19:25:53Z

Coverage Report ¹

Affected Products

`firebase-appdistribution`

Overall coverage changed from ? (56e2e2d) to 76.64% (8110244) by ?.

42 individual files with coverage change

Filename	Base (`56e2e2d`)	Merge (`8110244`)	Diff
AabUpdater.java	?	98.67%	?
ApkInstaller.java	?	96.88%	?
ApkUpdater.java	?	94.44%	?
AppDistributionReleaseImpl.java	?	100.00%	?
AppDistributionReleaseInternal.java	?	100.00%	?
AppIconSource.java	?	84.62%	?
AutoValue_AppDistributionReleaseImpl.java	?	65.45%	?
AutoValue_AppDistributionReleaseInternal.java	?	71.58%	?
AutoValue_ImageUtils_ImageSize.java	?	35.00%	?
AutoValue_TesterApiDisabledErrorDetails.java	?	29.41%	?
AutoValue_TesterApiDisabledErrorDetails_HelpLink.java	?	54.17%	?
AutoValue_UpdateProgressImpl.java	?	65.96%	?
ErrorMessages.java	?	0.00%	?
FeedbackActivity.java	?	1.12%	?
FeedbackSender.java	?	84.62%	?
FirebaseAppDistributionExceptions.java	?	80.00%	?
FirebaseAppDistributionFileProvider.java	?	0.00%	?
FirebaseAppDistributionImpl.java	?	89.77%	?
FirebaseAppDistributionLifecycleNotifier.java	?	92.91%	?
FirebaseAppDistributionNotificationsManager.java	?	87.18%	?
FirebaseAppDistributionRegistrar.java	?	86.27%	?
FirebaseAppDistributionTesterApiClient.java	?	88.54%	?
HttpsUrlConnectionFactory.java	?	50.00%	?
ImageUtils.java	?	100.00%	?
InstallActivity.java	?	2.67%	?
LogWrapper.java	?	100.00%	?
NewReleaseFetcher.java	?	80.85%	?
PackageInfoUtils.java	?	42.86%	?
ReleaseIdentifier.java	?	85.54%	?
ReleaseUtils.java	?	83.33%	?
ScreenshotTaker.java	?	41.18%	?
SequentialReference.java	?	100.00%	?
SignInResultActivity.java	?	0.00%	?
SignInStorage.java	?	42.86%	?
TakeScreenshotAndStartFeedbackActivity.java	?	0.00%	?
TaskCache.java	?	100.00%	?
TaskUtils.java	?	91.43%	?
TesterApiDisabledErrorDetails.java	?	93.75%	?
TesterApiHttpClient.java	?	90.18%	?
TesterSignInManager.java	?	93.55%	?
UpdateProgressImpl.java	?	100.00%	?
UpdateTaskImpl.java	?	76.00%	?

Test Logs

Merge (8110244): https://github.com/firebase/firebase-android-sdk/actions/runs/3744531952

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

lfkellogg · 2022-11-21T19:28:38Z

...tion/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java

@@ -65,7 +65,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
  private final SignInStorage signInStorage;
  private final ReleaseIdentifier releaseIdentifier;
  private final ScreenshotTaker screenshotTaker;
-  private final Executor taskExecutor;
+  private final Executor blockingExecutor;


Requires blocking executor here because some of the uses may wait on a lock via synchronized blocks.

@vkryachko Some of the uses in this file do not block at all. Would you recommend that we also inject the Lightweight executor, or is it up to us if we're OK using the Blocking executor for lighter weight tasks?

may wait on a lock via synchronized blocks

Do any of these blocks do network or disk IO synchronously or call task.get()? or are they all in memory operations?

(optional)

Additionally, speaking of synchronized blocks in the context of tasks and executors, consider refactoring to something like the following go/android-concurrent/whichexecutor#sequentialexecutor, where code that needs synchronization could run on the sequential "decorator", and code that does not can run on the executor directly.

The caveat is that we can't use ListenableFutures as described in the doc above and have to use Tasks instead. see TaskCompletionSource on how to do it.

I've also noticed that for some continuations in this file you are not passing in the executor, i.e. onSuccessTask, continueWithTask etc. This results in the continuations to run on the main thread, is that intentional?

Do any of these blocks do network or disk IO synchronously or call task.get()? or are they all in memory operations?

Actually looking closer .signInTester() accesses SharedPreferences, which I understand could in theory touch disk.

Disk it touched only when the SharedPreferences object is constructed, and for any synchronous writes like commit, so those operations should use the @Background executor as using an unbounded pool(i.e. @Blocking) for disk io puts a lot of pressure on the file system.

Besides that the other potential blocking calls should all be in memory. They involve synchronized blocks (like this one), which in theory shouldn't block for long if at all, but could block briefly depending on various race conditions.

In this case it's best to use a blocking executor since it's unbounded in the number of threads which avoids dead locks, but it's better to refactor to a sequential as mentioned below.

I assume logging using android.util.Log does not block but maybe that's wishful thinking...

It's safe to log and it does not block.

Additionally, speaking of synchronized blocks in the context of tasks and executors, consider refactoring to something like the following go/android-concurrent/whichexecutor#sequentialexecutor

That looks great, we'll look into that. It doesn't feel great having synchronized blocks all over the place right now.

I've also noticed that for some continuations in this file you are not passing in the executor, i.e. onSuccessTask, continueWithTask etc. This results in the continuations to run on the main thread, is that intentional?

I see a couple of them are accidental, actually. I'll fix those. Others though are intentional, if not ideal. We run a lot of Task handlers on the main thread if they should execute quickly. But that's mostly for convenience, rather than create an executor to run something that should return immediately. Maybe in this new world of explicit shared executors we should use @Lightweight for these.

In my opinion this is a bad default that Tasks provide(even though at the time it was a reasonable thing to do), the general android guidance is to run as little code on the UI thread as possible, i.e. only things that must run there, like UI related code. Given this default, I agree that using executors explicitly in all continuations is a better approach and we should even add a linter warning for calls without an executor(maybe?).

OK thanks Vlad, in summary we will:

Move construction of SharedPreferences or synchronous writes to a @Background executor

Avoid using synchronized blocks in favor of sequential executors or other approaches (already removed some in Remove some synchronized blocks from FirebaseAppDistributionImpl #4353)

Use explicit executors for Task handlers throughout instead of defaulting to UI thread

Thanks Lee,

Move construction of SharedPreferences or synchronous writes to a @Background executor

One thing I'll add: it would be awesome if you could implement this as a self-contained class that just does the right thing, that way we would be able to put it in say firebase-common, register it as a component, and have all teams benefit from it. wdyt?

As for the UI thread, looks like all sdks use it a lot :( #4363

So it could be a follow up effort once the executor migration is done

Sounds good. I'm going to make each of these changes in followup PRs.

google-oss-bot · 2022-11-21T19:38:23Z

Size Report ¹

Affected Products

base
Type Base (56e2e2d) Merge (8110244) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.65 kB ? (?)
firebase-annotations
Type Base (56e2e2d) Merge (8110244) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 9.46 kB ? (?)
firebase-appdistribution
Type Base (56e2e2d) Merge (8110244) Diff
aar ? 150 kB ? (?)
apk (aggressive) ? 832 kB ? (?)
apk (release) ? 2.01 MB ? (?)
firebase-appdistribution-api
Type Base (56e2e2d) Merge (8110244) Diff
aar ? 16.0 kB ? (?)
apk (aggressive) ? 95.8 kB ? (?)
apk (release) ? 706 kB ? (?)
firebase-common
Type Base (56e2e2d) Merge (8110244) Diff
aar ? 67.4 kB ? (?)
apk (aggressive) ? 95.1 kB ? (?)
apk (release) ? 700 kB ? (?)
firebase-components
Type Base (56e2e2d) Merge (8110244) Diff
aar ? 44.9 kB ? (?)
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 33.6 kB ? (?)
firebase-installations
Type Base (56e2e2d) Merge (8110244) Diff
aar ? 55.0 kB ? (?)
apk (aggressive) ? 96.6 kB ? (?)
apk (release) ? 724 kB ? (?)
firebase-installations-interop
Type Base (56e2e2d) Merge (8110244) Diff
aar ? 8.05 kB ? (?)
apk (aggressive) ? 65.0 kB ? (?)
apk (release) ? 651 kB ? (?)

Test Logs

Merge (8110244): https://github.com/firebase/firebase-android-sdk/actions/runs/3744531952

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

kaibolay · 2022-11-21T20:05:10Z

...-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java

-      FirebaseApp firebaseApp, FirebaseAppDistributionLifecycleNotifier lifecycleNotifier) {
-    this(firebaseApp, lifecycleNotifier, Executors.newSingleThreadExecutor());
-  }
+  private final Executor backgroundExecutor;

  @VisibleForTesting


remove this annotation - this constructor is now called from FirebaseAppDistributionRegistrar

Ah good catch

github-actions · 2022-12-20T19:15:47Z

Unit Test Results

161 tests 161 ✔️ 40s ⏱️
  16 suites     0 💤
  16 files     0 ❌

Results for commit 8e75ef4.

♻️ This comment has been updated with latest results.

google-oss-bot · 2022-12-20T22:32:55Z

Startup Time Report ¹

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

Startup time comparison between the CI merge commit (8110244) and the base commit (56e2e2d) are not available.

No macrobenchmark data found for the base commit (56e2e2d). Analysis for the CI merge commit (8110244) can be found at:

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

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

* Migrate executors in fad/in-app-feedback * Add an import and format

lfkellogg requested review from vkryachko and kaibolay November 21, 2022 19:19

google-oss-bot added the size/M label Nov 21, 2022

lfkellogg commented Nov 21, 2022

View reviewed changes

kaibolay approved these changes Nov 21, 2022

View reviewed changes

lfkellogg force-pushed the lk/migrate-iaf-executors branch from 48d57ce to c9cf94d Compare December 7, 2022 15:02

lfkellogg force-pushed the fad/in-app-feedback branch from e77f3c8 to 233dead Compare December 7, 2022 15:04

lfkellogg force-pushed the lk/migrate-iaf-executors branch from c9cf94d to fe558c7 Compare December 7, 2022 15:10

lfkellogg mentioned this pull request Dec 8, 2022

Use sequential executor in place of synchronized blocks for cached new release #4430

Merged

lfkellogg force-pushed the fad/in-app-feedback branch from 233dead to 6323c25 Compare December 9, 2022 18:34

lfkellogg force-pushed the fad/in-app-feedback branch from 03b7b9e to d545fc8 Compare December 20, 2022 15:44

Migrate executors in fad/in-app-feedback

ec07e81

lfkellogg force-pushed the lk/migrate-iaf-executors branch from fe558c7 to ec07e81 Compare December 20, 2022 19:09

Add an import and format

8e75ef4

lfkellogg mentioned this pull request Dec 20, 2022

Move SharedPreferences usage to background thread #4480

Merged

lfkellogg changed the title ~~Migrate executors in fad/in-app-feedback~~ Migrate some executors in fad/in-app-feedback Dec 20, 2022

lfkellogg merged commit 8a08968 into fad/in-app-feedback Dec 20, 2022

lfkellogg deleted the lk/migrate-iaf-executors branch December 20, 2022 22:23

lfkellogg added a commit that referenced this pull request Jan 5, 2023

Migrate some executors in fad/in-app-feedback (#4350)

728b530

* Migrate executors in fad/in-app-feedback * Add an import and format

firebase locked and limited conversation to collaborators Jan 20, 2023

Migrate some executors in fad/in-app-feedback #4350

Migrate some executors in fad/in-app-feedback #4350

Uh oh!

Conversation

lfkellogg commented Nov 21, 2022

Uh oh!

google-oss-bot commented Nov 21, 2022

Uh oh!

google-oss-bot commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-appdistribution

Test Logs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkryachko Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkryachko Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

base

firebase-annotations

firebase-appdistribution

firebase-appdistribution-api

firebase-common

firebase-components

firebase-installations

firebase-installations-interop

Test Logs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

google-oss-bot commented Dec 20, 2022

Startup Time Report 1

Uh oh!

Uh oh!

google-oss-bot commented Nov 21, 2022 •

edited

Loading

Coverage Report ¹

`firebase-appdistribution`

vkryachko Nov 22, 2022 •

edited

Loading

vkryachko Nov 24, 2022 •

edited

Loading

google-oss-bot commented Nov 21, 2022 •

edited

Loading

Size Report ¹

`base`

`firebase-annotations`

`firebase-appdistribution`

`firebase-appdistribution-api`

`firebase-common`

`firebase-components`

`firebase-installations`

`firebase-installations-interop`

github-actions bot commented Dec 20, 2022 •

edited

Loading

Startup Time Report ¹