-
Notifications
You must be signed in to change notification settings - Fork 641
firestore: replace deprecated AsyncTask-based executor with modern Kotlin dispatchers #7383
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -237,7 +237,7 @@ private void initChannelTask() { | |||||
// the AsyncQueue. | ||||||
this.channelTask = | ||||||
Tasks.call( | ||||||
Executors.BACKGROUND_EXECUTOR, | ||||||
Executors.SHORT_WORKLOAD_EXECUTOR, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||||
() -> { | ||||||
ManagedChannel channel = initChannel(context, databaseInfo); | ||||||
asyncQueue.enqueueAndForget(() -> onConnectivityStateChange(channel)); | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -14,20 +14,23 @@ | |||||||||||||||||
|
||||||||||||||||||
package com.google.firebase.firestore.util; | ||||||||||||||||||
|
||||||||||||||||||
import android.os.AsyncTask; | ||||||||||||||||||
import static kotlinx.coroutines.ExecutorsKt.asExecutor; | ||||||||||||||||||
|
||||||||||||||||||
import com.google.android.gms.tasks.TaskExecutors; | ||||||||||||||||||
import java.util.concurrent.Executor; | ||||||||||||||||||
import kotlinx.coroutines.Dispatchers; | ||||||||||||||||||
|
||||||||||||||||||
/** Helper class for executors. */ | ||||||||||||||||||
public final class Executors { | ||||||||||||||||||
/** | ||||||||||||||||||
* The maximum number of tasks we submit to AsyncTask.THREAD_POOL_EXECUTOR. | ||||||||||||||||||
* | ||||||||||||||||||
* <p>The limit is based on the number of core threads spun by THREAD_POOL_EXECUTOR and is well | ||||||||||||||||||
* below the queue size limit of 120 pending tasks. Limiting our usage of the THREAD_POOL_EXECUTOR | ||||||||||||||||||
* allows other users to schedule their own operations on the shared THREAD_POOL_EXECUTOR. | ||||||||||||||||||
* The number of physical CPU cores available for multithreaded execution, or 2, whichever is | ||||||||||||||||||
* larger. | ||||||||||||||||||
* <p> | ||||||||||||||||||
* CPU-bound tasks should never use more than this number of concurrent threads as doing so will | ||||||||||||||||||
* almost certainly reduce throughput due to the overhead of context switching. | ||||||||||||||||||
*/ | ||||||||||||||||||
private static final int ASYNC_THREAD_POOL_MAXIMUM_CONCURRENCY = 4; | ||||||||||||||||||
public static final int HARDWARE_CONCURRENCY = | ||||||||||||||||||
Math.max(2, Runtime.getRuntime().availableProcessors()); | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* The default executor for user visible callbacks. It is an executor scheduling callbacks on | ||||||||||||||||||
|
@@ -38,10 +41,46 @@ public final class Executors { | |||||||||||||||||
/** An executor that executes the provided runnable immediately on the current thread. */ | ||||||||||||||||||
public static final Executor DIRECT_EXECUTOR = Runnable::run; | ||||||||||||||||||
|
||||||||||||||||||
/** An executor that runs tasks in parallel on Android's AsyncTask.THREAD_POOL_EXECUTOR. */ | ||||||||||||||||||
public static final Executor BACKGROUND_EXECUTOR = | ||||||||||||||||||
new ThrottledForwardingExecutor( | ||||||||||||||||||
ASYNC_THREAD_POOL_MAXIMUM_CONCURRENCY, AsyncTask.THREAD_POOL_EXECUTOR); | ||||||||||||||||||
/** | ||||||||||||||||||
* An executor suitable for short tasks that perform little or no blocking. | ||||||||||||||||||
*/ | ||||||||||||||||||
public static final Executor SHORT_WORKLOAD_EXECUTOR = | ||||||||||||||||||
asExecutor( | ||||||||||||||||||
Dispatchers.getIO() | ||||||||||||||||||
.limitedParallelism(HARDWARE_CONCURRENCY, "firestore.SHORT_WORKLOAD_EXECUTOR")); | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* An executor suitable for IO-bound workloads. New threads are usually created to satisfy demand, | ||||||||||||||||||
* and, therefore, tasks do not usually wait in a queue for execution. | ||||||||||||||||||
*/ | ||||||||||||||||||
public static final Executor IO_WORKLOAD_EXECUTOR = asExecutor(Dispatchers.getIO()); | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* An executor suitable for CPU-bound workloads. No more tasks than available CPU cores will | ||||||||||||||||||
* execute concurrently, while other tasks line up and wait for a thread to become available, and | ||||||||||||||||||
* are scheduled in an arbitrary order. | ||||||||||||||||||
*/ | ||||||||||||||||||
public static final Executor CPU_WORKLOAD_EXECUTOR = | ||||||||||||||||||
asExecutor( | ||||||||||||||||||
Dispatchers.getIO() | ||||||||||||||||||
.limitedParallelism(HARDWARE_CONCURRENCY, "firestore.CPU_WORKLOAD_EXECUTOR")); | ||||||||||||||||||
Comment on lines
+63
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a couple of issues with this executor definition:
I suggest changing
Suggested change
|
||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* Creates and returns a new {@link Executor} that executes tasks sequentially. | ||||||||||||||||||
* <p> | ||||||||||||||||||
* The implementation guarantees that tasks are executed sequentially and that a happens-before | ||||||||||||||||||
* relation is established between them. This means that tasks run by this executor do _not_ need | ||||||||||||||||||
* to synchronize access to shared resources, such as using "synchronized" blocks or "volatile" | ||||||||||||||||||
* variables. See `kotlinx.coroutines.limitedParallelism` for full details. | ||||||||||||||||||
* <p> | ||||||||||||||||||
* Note that there is no guarantee that tasks will all run on the _same thread_. | ||||||||||||||||||
* | ||||||||||||||||||
* @param name a brief name to assign to the executor, for debugging purposes. | ||||||||||||||||||
* @return the newly-created executor. | ||||||||||||||||||
*/ | ||||||||||||||||||
public static Executor newSequentialExecutor(String name) { | ||||||||||||||||||
return asExecutor(Dispatchers.getIO().limitedParallelism(1, "firestore.seq." + name)); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
private Executors() { | ||||||||||||||||||
// Private constructor to prevent initialization | ||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo here ('mordern'). Also, please remember to replace the
[#NNNN]
placeholder on the next line with the actual issue or pull request number.