From 0255bf90de37cd4e9e19a16ace11038f5972d8c6 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 15 Sep 2025 15:04:59 -0400 Subject: [PATCH 1/6] firestore: re-write BackgroundQueue to NOT use the deprecated AsyncTask thread pool This change results in a significant performance improvement of queries with a large number of documents in their result set. In my testing, the performance improved by 91% from 3022 ms down to 265 ms on a real device! For some reason, there were _no_ performance gains on the Android emulator. This must have something to do with how different Android OS versions manage the threads in the (deprecated) AsyncTask thread pool. The test environment was as follows: - Pixel 7 Pro real device running Android 16. - Test called collectionRef.whereGreaterThan("foo", 50).get(Source.CACHE) 200 times. - Calculated the average amount of time it took for the query results to be received. - collectionRef is a Firestore collection containing 10,000 documents, of which 50% match the "whereGreaterThan" filter. - Local cache had no other documents in it. - Test app was compiled in "release" mode with "proguard-android-optimize.txt" r8 configuration. --- firebase-firestore/CHANGELOG.md | 3 + .../local/SQLiteDocumentOverlayCache.java | 7 +- .../local/SQLiteRemoteDocumentCache.java | 7 +- .../firestore/util/BackgroundQueue.java | 54 ------------ .../firestore/util/BackgroundQueue.kt | 88 +++++++++++++++++++ .../firebase/firestore/core/QueryTest.java | 7 +- 6 files changed, 94 insertions(+), 72 deletions(-) delete mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.java create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index d2c68fbc5cf..de24f557c1d 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -3,6 +3,9 @@ - [changed] Bumped internal dependencies. - [changed] Improve the performance of queries in collections that contain many deleted documents. [#7295](//github.com/firebase/firebase-android-sdk/issues/7295) +- [changed] Improve query performance in large result sets by replacing the deprecated AsyncTask + thread pool with a self-managed thread pool. + [#NNNN](//github.com/firebase/firebase-android-sdk/issues/NNNN) # 26.0.0 diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java index 84628edd4a2..53dc6415677 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java @@ -26,7 +26,6 @@ import com.google.firebase.firestore.model.mutation.Mutation; import com.google.firebase.firestore.model.mutation.Overlay; import com.google.firebase.firestore.util.BackgroundQueue; -import com.google.firebase.firestore.util.Executors; import com.google.firestore.v1.Write; import com.google.protobuf.InvalidProtocolBufferException; import java.util.ArrayList; @@ -35,7 +34,6 @@ import java.util.List; import java.util.Map; import java.util.SortedSet; -import java.util.concurrent.Executor; public class SQLiteDocumentOverlayCache implements DocumentOverlayCache { private final SQLitePersistence db; @@ -204,10 +202,7 @@ private void processOverlaysInBackground( byte[] rawMutation = row.getBlob(0); int largestBatchId = row.getInt(1); - // Since scheduling background tasks incurs overhead, we only dispatch to a - // background thread if there are still some documents remaining. - Executor executor = row.isLast() ? Executors.DIRECT_EXECUTOR : backgroundQueue; - executor.execute( + backgroundQueue.submit( () -> { Overlay overlay = decodeOverlay(rawMutation, largestBatchId); synchronized (results) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 5690a79ae70..2c1a4dcdf88 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -33,7 +33,6 @@ import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.util.BackgroundQueue; -import com.google.firebase.firestore.util.Executors; import com.google.firebase.firestore.util.Function; import com.google.protobuf.InvalidProtocolBufferException; import com.google.protobuf.MessageLite; @@ -47,7 +46,6 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.Executor; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -308,10 +306,7 @@ private void processRowInBackground( boolean documentTypeIsNull = row.isNull(3); String path = row.getString(4); - // Since scheduling background tasks incurs overhead, we only dispatch to a - // background thread if there are still some documents remaining. - Executor executor = row.isLast() ? Executors.DIRECT_EXECUTOR : backgroundQueue; - executor.execute( + backgroundQueue.submit( () -> { MutableDocument document = decodeMaybeDocument(rawDocument, readTimeSeconds, readTimeNanos); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.java deleted file mode 100644 index 5a443c7963b..00000000000 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.java +++ /dev/null @@ -1,54 +0,0 @@ -// Copyright 2019 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.firebase.firestore.util; - -import static com.google.firebase.firestore.util.Assert.fail; - -import java.util.concurrent.Executor; -import java.util.concurrent.Semaphore; - -/** - * A simple queue that executes tasks in parallel on the Android's AsyncTask.THREAD_POOL_EXECUTOR - * and supports blocking on their completion. - * - *

This class is not thread-safe. In particular, `execute()` and `drain()` should not be called - * from parallel threads. - */ -public class BackgroundQueue implements Executor { - private Semaphore completedTasks = new Semaphore(0); - private int pendingTaskCount = 0; - - /** Enqueue a task on Android's THREAD_POOL_EXECUTOR. */ - @Override - public void execute(Runnable task) { - ++pendingTaskCount; - Executors.BACKGROUND_EXECUTOR.execute( - () -> { - task.run(); - completedTasks.release(); - }); - } - - /** Wait for all currently scheduled tasks to complete. */ - public void drain() { - try { - completedTasks.acquire(pendingTaskCount); - pendingTaskCount = 0; - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - fail("Interrupted while waiting for background task", e); - } - } -} diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt new file mode 100644 index 00000000000..9e8cde8b6ca --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt @@ -0,0 +1,88 @@ +/* + * Copyright 2025 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.firebase.firestore.util + +import java.util.concurrent.Executor +import java.util.concurrent.Semaphore +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.asExecutor + +/** + * Manages CPU-bound work on background threads to enable parallel processing. + * + * Instances of this class are _not_ thread-safe. All methods of an instance of this class must be + * called from the same thread. The behavior of an instance is undefined if any of the methods are + * called from multiple threads. + */ +internal class BackgroundQueue { + + private var state: State = State.Submitting() + + /** + * Submit a task for asynchronous execution on the executor of the owning [BackgroundQueue]. + * + * @throws IllegalStateException if [drain] has been called. + */ + fun submit(runnable: Runnable) { + val submittingState = this.state + check(submittingState is State.Submitting) { "submit() may not be called after drain()" } + + submittingState.run { + taskCount++ + executor.execute { + try { + runnable.run() + } finally { + completedTasks.release() + } + } + } + } + + /** + * Blocks until all tasks submitted via calls to [submit] have completed. + * + * @throws IllegalStateException if called more than once. + */ + fun drain() { + val submittingState = this.state + check(submittingState is State.Submitting) { "drain() may not be called more than once" } + this.state = State.Draining + + submittingState.run { completedTasks.acquire(taskCount) } + } + + private sealed interface State { + class Submitting : State { + val completedTasks = Semaphore(0) + var taskCount: Int = 0 + } + object Draining : State + } + + companion object { + + /** + * The maximum amount of parallelism shared by all instances of this class. + * + * This is equal to the number of processor cores available, or 2, whichever is larger. + */ + val maxParallelism = Runtime.getRuntime().availableProcessors().coerceAtLeast(2) + + private val executor: Executor = + Dispatchers.IO.limitedParallelism(maxParallelism, "firestore.BackgroundQueue").asExecutor() + } +} diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java index cdc932bfa01..7580be68ad9 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java @@ -40,14 +40,12 @@ import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.testutil.ComparatorTester; import com.google.firebase.firestore.util.BackgroundQueue; -import com.google.firebase.firestore.util.Executors; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.concurrent.Executor; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -991,10 +989,7 @@ public void testSynchronousMatchesOrderBy() { while (iterator.hasNext()) { MutableDocument doc = iterator.next(); - // Only put the processing in the backgroundQueue if there are more documents - // in the list. This behavior matches SQLiteRemoteDocumentCache.getAll(...) - Executor executor = iterator.hasNext() ? backgroundQueue : Executors.DIRECT_EXECUTOR; - executor.execute( + backgroundQueue.submit( () -> { // We call query.matches() to indirectly test query.matchesOrderBy() boolean result = query.matches(doc); From 4f45dbd22b47cab5bd344bda7021a8f6e220644d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 15 Sep 2025 15:39:14 -0400 Subject: [PATCH 2/6] CHANGELOG.md: update PR number --- firebase-firestore/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index de24f557c1d..b11800c1e00 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -5,7 +5,7 @@ [#7295](//github.com/firebase/firebase-android-sdk/issues/7295) - [changed] Improve query performance in large result sets by replacing the deprecated AsyncTask thread pool with a self-managed thread pool. - [#NNNN](//github.com/firebase/firebase-android-sdk/issues/NNNN) + [#7376](//github.com/firebase/firebase-android-sdk/issues/7376) # 26.0.0 From 32c3addc38d124f4a126adac8b2c3d4217b076ae Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 15 Sep 2025 17:06:49 -0400 Subject: [PATCH 3/6] use threadlocal instead of synchronization --- .../local/SQLiteDocumentOverlayCache.java | 47 +++++++------- .../local/SQLiteRemoteDocumentCache.java | 34 ++++------ .../firestore/util/BackgroundQueue.kt | 62 +++++++++++++++---- .../firebase/firestore/core/QueryTest.java | 15 ++--- 4 files changed, 87 insertions(+), 71 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java index 53dc6415677..fc114ba0c0b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java @@ -61,29 +61,26 @@ public Overlay getOverlay(DocumentKey key) { @Override public Map getOverlays(SortedSet keys) { hardAssert(keys.comparator() == null, "getOverlays() requires natural order"); - Map result = new HashMap<>(); - BackgroundQueue backgroundQueue = new BackgroundQueue(); + BackgroundQueue backgroundQueue = new BackgroundQueue<>(); ResourcePath currentCollection = ResourcePath.EMPTY; List accumulatedDocumentIds = new ArrayList<>(); for (DocumentKey key : keys) { if (!currentCollection.equals(key.getCollectionPath())) { - processSingleCollection(result, backgroundQueue, currentCollection, accumulatedDocumentIds); + processSingleCollection(backgroundQueue, currentCollection, accumulatedDocumentIds); currentCollection = key.getCollectionPath(); accumulatedDocumentIds.clear(); } accumulatedDocumentIds.add(key.getDocumentId()); } - processSingleCollection(result, backgroundQueue, currentCollection, accumulatedDocumentIds); - backgroundQueue.drain(); - return result; + processSingleCollection(backgroundQueue, currentCollection, accumulatedDocumentIds); + return backgroundQueue.drain(); } /** Reads the overlays for the documents in a single collection. */ private void processSingleCollection( - Map result, - BackgroundQueue backgroundQueue, + BackgroundQueue backgroundQueue, ResourcePath collectionPath, List documentIds) { if (documentIds.isEmpty()) { @@ -101,7 +98,7 @@ private void processSingleCollection( while (longQuery.hasMoreSubqueries()) { longQuery .performNextSubquery() - .forEach(row -> processOverlaysInBackground(backgroundQueue, result, row)); + .forEach(row -> processOverlaysInBackground(backgroundQueue, row)); } } @@ -138,26 +135,23 @@ public void removeOverlaysForBatchId(int batchId) { @Override public Map getOverlays(ResourcePath collection, int sinceBatchId) { - Map result = new HashMap<>(); - BackgroundQueue backgroundQueue = new BackgroundQueue(); + BackgroundQueue backgroundQueue = new BackgroundQueue<>(); db.query( "SELECT overlay_mutation, largest_batch_id FROM document_overlays " + "WHERE uid = ? AND collection_path = ? AND largest_batch_id > ?") .binding(uid, EncodedPath.encode(collection), sinceBatchId) - .forEach(row -> processOverlaysInBackground(backgroundQueue, result, row)); - backgroundQueue.drain(); - return result; + .forEach(row -> processOverlaysInBackground(backgroundQueue, row)); + return backgroundQueue.drain(); } @Override public Map getOverlays( String collectionGroup, int sinceBatchId, int count) { - Map result = new HashMap<>(); String[] lastCollectionPath = new String[1]; String[] lastDocumentPath = new String[1]; int[] lastLargestBatchId = new int[1]; - BackgroundQueue backgroundQueue = new BackgroundQueue(); + BackgroundQueue backgroundQueue1 = new BackgroundQueue<>(); db.query( "SELECT overlay_mutation, largest_batch_id, collection_path, document_id " + " FROM document_overlays " @@ -169,17 +163,19 @@ public Map getOverlays( lastLargestBatchId[0] = row.getInt(1); lastCollectionPath[0] = row.getString(2); lastDocumentPath[0] = row.getString(3); - processOverlaysInBackground(backgroundQueue, result, row); + processOverlaysInBackground(backgroundQueue1, row); }); + HashMap results = backgroundQueue1.drain(); if (lastCollectionPath[0] == null) { - return result; + return results; } // This function should not return partial batch overlays, even if the number of overlays in the // result set exceeds the given `count` argument. Since the `LIMIT` in the above query might // result in a partial batch, the following query appends any remaining overlays for the last // batch. + BackgroundQueue backgroundQueue2 = new BackgroundQueue<>(); db.query( "SELECT overlay_mutation, largest_batch_id FROM document_overlays " + "WHERE uid = ? AND collection_group = ? " @@ -192,22 +188,21 @@ public Map getOverlays( lastCollectionPath[0], lastDocumentPath[0], lastLargestBatchId[0]) - .forEach(row -> processOverlaysInBackground(backgroundQueue, result, row)); - backgroundQueue.drain(); - return result; + .forEach(row -> processOverlaysInBackground(backgroundQueue2, row)); + + backgroundQueue2.drainInto(results); + return results; } private void processOverlaysInBackground( - BackgroundQueue backgroundQueue, Map results, Cursor row) { + BackgroundQueue backgroundQueue, Cursor row) { byte[] rawMutation = row.getBlob(0); int largestBatchId = row.getInt(1); backgroundQueue.submit( - () -> { + results -> { Overlay overlay = decodeOverlay(rawMutation, largestBatchId); - synchronized (results) { - results.put(overlay.getKey(), overlay); - } + results.put(overlay.getKey(), overlay); }); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 2c1a4dcdf88..7450b4a10c2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -168,21 +168,19 @@ public Map getAll(Iterable documentKe bindVars, ") ORDER BY path"); - BackgroundQueue backgroundQueue = new BackgroundQueue(); + BackgroundQueue backgroundQueue = new BackgroundQueue<>(); while (longQuery.hasMoreSubqueries()) { longQuery .performNextSubquery() - .forEach(row -> processRowInBackground(backgroundQueue, results, row, /*filter*/ null)); + .forEach(row -> processRowInBackground(backgroundQueue, row, /*filter*/ null)); } - backgroundQueue.drain(); + + backgroundQueue.drainInto(results); // Backfill any rows with null "document_type" discovered by processRowInBackground(). documentTypeBackfiller.backfill(db); - // Synchronize on `results` to avoid a data race with the background queue. - synchronized (results) { - return results; - } + return results; } @Override @@ -264,26 +262,23 @@ private Map getAll( } bindVars[i] = count; - BackgroundQueue backgroundQueue = new BackgroundQueue(); - Map results = new HashMap<>(); + BackgroundQueue backgroundQueue = new BackgroundQueue<>(); db.query(sql.toString()) .binding(bindVars) .forEach( row -> { - processRowInBackground(backgroundQueue, results, row, filter); + processRowInBackground(backgroundQueue, row, filter); if (context != null) { context.incrementDocumentReadCount(); } }); - backgroundQueue.drain(); + + HashMap results = backgroundQueue.drain(); // Backfill any null "document_type" columns discovered by processRowInBackground(). documentTypeBackfiller.backfill(db); - // Synchronize on `results` to avoid a data race with the background queue. - synchronized (results) { - return results; - } + return results; } private Map getAll( @@ -296,8 +291,7 @@ private Map getAll( } private void processRowInBackground( - BackgroundQueue backgroundQueue, - Map results, + BackgroundQueue backgroundQueue, Cursor row, @Nullable Function filter) { byte[] rawDocument = row.getBlob(0); @@ -307,16 +301,14 @@ private void processRowInBackground( String path = row.getString(4); backgroundQueue.submit( - () -> { + results -> { MutableDocument document = decodeMaybeDocument(rawDocument, readTimeSeconds, readTimeNanos); if (documentTypeIsNull) { documentTypeBackfiller.enqueue(path, readTimeSeconds, readTimeNanos, document); } if (filter == null || filter.apply(document)) { - synchronized (results) { - results.put(document.getKey(), document); - } + results.put(document.getKey(), document); } }); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt index 9e8cde8b6ca..16cd0979d21 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt @@ -27,24 +27,29 @@ import kotlinx.coroutines.asExecutor * called from the same thread. The behavior of an instance is undefined if any of the methods are * called from multiple threads. */ -internal class BackgroundQueue { +internal class BackgroundQueue { - private var state: State = State.Submitting() + private var state: State = State.Submitting() + + /** Overload for convenience of being called from Java code. */ + fun submit(consumer: Consumer>) = this.submit { consumer.accept(it) } /** * Submit a task for asynchronous execution on the executor of the owning [BackgroundQueue]. * * @throws IllegalStateException if [drain] has been called. */ - fun submit(runnable: Runnable) { + fun submit(runnable: (results: HashMap) -> Unit) { val submittingState = this.state - check(submittingState is State.Submitting) { "submit() may not be called after drain()" } + check(submittingState is State.Submitting) { + "submit() may not be called after drain() or drainInto()" + } submittingState.run { taskCount++ executor.execute { try { - runnable.run() + runnable(threadLocalResults.get()!!) } finally { completedTasks.release() } @@ -55,22 +60,53 @@ internal class BackgroundQueue { /** * Blocks until all tasks submitted via calls to [submit] have completed. * - * @throws IllegalStateException if called more than once. + * The results produced by each thread are merged into a new [HashMap] and returned. + * + * @throws IllegalStateException if [drain] or [drainInto] has already been called. + */ + fun drain(): HashMap = HashMap().also { drainInto(it) } + + /** + * Blocks until all tasks submitted via calls to [submit] have completed. + * + * The results produced by each thread are merged into the given map. + * + * @throws IllegalStateException if [drain] or [drainInto] has already been called. */ - fun drain() { + fun drainInto(results: MutableMap) { val submittingState = this.state - check(submittingState is State.Submitting) { "drain() may not be called more than once" } - this.state = State.Draining + check(submittingState is State.Submitting) { "drain() or drainInto() has already been called" } + this.state = State.Draining() + return submittingState.run { + completedTasks.acquire(taskCount) + threadLocalResults.mergeResultsInto(results) + } + } + + private class ThreadLocalResults : ThreadLocal>() { + + private val results = mutableListOf>() - submittingState.run { completedTasks.acquire(taskCount) } + override fun initialValue(): HashMap { + synchronized(results) { + val result = HashMap() + results.add(result) + return result + } + } + + fun mergeResultsInto(mergedResults: MutableMap) { + synchronized(results) { results.forEach { mergedResults.putAll(it) } } + } } - private sealed interface State { - class Submitting : State { + private sealed interface State { + class Submitting : State { val completedTasks = Semaphore(0) + val threadLocalResults = ThreadLocalResults() var taskCount: Int = 0 } - object Draining : State + class Draining : State } companion object { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java index 7580be68ad9..7189bf3747e 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java @@ -45,7 +45,6 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; -import java.util.Map; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -984,25 +983,19 @@ public void testSynchronousMatchesOrderBy() { // SQLiteRemoteDocumentCache.getAll(...), where `query.matches(doc)` is performed // for many different docs concurrently on the BackgroundQueue. Iterator iterator = docs.iterator(); - BackgroundQueue backgroundQueue = new BackgroundQueue(); - Map results = new HashMap<>(); + BackgroundQueue backgroundQueue = new BackgroundQueue<>(); while (iterator.hasNext()) { MutableDocument doc = iterator.next(); backgroundQueue.submit( - () -> { + results -> { // We call query.matches() to indirectly test query.matchesOrderBy() boolean result = query.matches(doc); - - // We will include a synchronized block in our command to simulate - // the implementation in SQLiteRemoteDocumentCache.getAll(...) - synchronized (results) { - results.put(doc.getKey(), result); - } + results.put(doc.getKey(), result); }); } - backgroundQueue.drain(); + HashMap results = backgroundQueue.drain(); Assert.assertEquals(101, results.keySet().size()); for (DocumentKey key : results.keySet()) { From a9dc9769588a204d70181d5e6f80e3d7ce7a83d0 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 15 Sep 2025 17:06:53 -0400 Subject: [PATCH 4/6] Revert "use threadlocal instead of synchronization" This reverts commit 32c3addc38d124f4a126adac8b2c3d4217b076ae. It made no performance improvements whatsoever. --- .../local/SQLiteDocumentOverlayCache.java | 47 +++++++------- .../local/SQLiteRemoteDocumentCache.java | 34 ++++++---- .../firestore/util/BackgroundQueue.kt | 62 ++++--------------- .../firebase/firestore/core/QueryTest.java | 15 +++-- 4 files changed, 71 insertions(+), 87 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java index fc114ba0c0b..53dc6415677 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java @@ -61,26 +61,29 @@ public Overlay getOverlay(DocumentKey key) { @Override public Map getOverlays(SortedSet keys) { hardAssert(keys.comparator() == null, "getOverlays() requires natural order"); + Map result = new HashMap<>(); - BackgroundQueue backgroundQueue = new BackgroundQueue<>(); + BackgroundQueue backgroundQueue = new BackgroundQueue(); ResourcePath currentCollection = ResourcePath.EMPTY; List accumulatedDocumentIds = new ArrayList<>(); for (DocumentKey key : keys) { if (!currentCollection.equals(key.getCollectionPath())) { - processSingleCollection(backgroundQueue, currentCollection, accumulatedDocumentIds); + processSingleCollection(result, backgroundQueue, currentCollection, accumulatedDocumentIds); currentCollection = key.getCollectionPath(); accumulatedDocumentIds.clear(); } accumulatedDocumentIds.add(key.getDocumentId()); } - processSingleCollection(backgroundQueue, currentCollection, accumulatedDocumentIds); - return backgroundQueue.drain(); + processSingleCollection(result, backgroundQueue, currentCollection, accumulatedDocumentIds); + backgroundQueue.drain(); + return result; } /** Reads the overlays for the documents in a single collection. */ private void processSingleCollection( - BackgroundQueue backgroundQueue, + Map result, + BackgroundQueue backgroundQueue, ResourcePath collectionPath, List documentIds) { if (documentIds.isEmpty()) { @@ -98,7 +101,7 @@ private void processSingleCollection( while (longQuery.hasMoreSubqueries()) { longQuery .performNextSubquery() - .forEach(row -> processOverlaysInBackground(backgroundQueue, row)); + .forEach(row -> processOverlaysInBackground(backgroundQueue, result, row)); } } @@ -135,23 +138,26 @@ public void removeOverlaysForBatchId(int batchId) { @Override public Map getOverlays(ResourcePath collection, int sinceBatchId) { - BackgroundQueue backgroundQueue = new BackgroundQueue<>(); + Map result = new HashMap<>(); + BackgroundQueue backgroundQueue = new BackgroundQueue(); db.query( "SELECT overlay_mutation, largest_batch_id FROM document_overlays " + "WHERE uid = ? AND collection_path = ? AND largest_batch_id > ?") .binding(uid, EncodedPath.encode(collection), sinceBatchId) - .forEach(row -> processOverlaysInBackground(backgroundQueue, row)); - return backgroundQueue.drain(); + .forEach(row -> processOverlaysInBackground(backgroundQueue, result, row)); + backgroundQueue.drain(); + return result; } @Override public Map getOverlays( String collectionGroup, int sinceBatchId, int count) { + Map result = new HashMap<>(); String[] lastCollectionPath = new String[1]; String[] lastDocumentPath = new String[1]; int[] lastLargestBatchId = new int[1]; - BackgroundQueue backgroundQueue1 = new BackgroundQueue<>(); + BackgroundQueue backgroundQueue = new BackgroundQueue(); db.query( "SELECT overlay_mutation, largest_batch_id, collection_path, document_id " + " FROM document_overlays " @@ -163,19 +169,17 @@ public Map getOverlays( lastLargestBatchId[0] = row.getInt(1); lastCollectionPath[0] = row.getString(2); lastDocumentPath[0] = row.getString(3); - processOverlaysInBackground(backgroundQueue1, row); + processOverlaysInBackground(backgroundQueue, result, row); }); - HashMap results = backgroundQueue1.drain(); if (lastCollectionPath[0] == null) { - return results; + return result; } // This function should not return partial batch overlays, even if the number of overlays in the // result set exceeds the given `count` argument. Since the `LIMIT` in the above query might // result in a partial batch, the following query appends any remaining overlays for the last // batch. - BackgroundQueue backgroundQueue2 = new BackgroundQueue<>(); db.query( "SELECT overlay_mutation, largest_batch_id FROM document_overlays " + "WHERE uid = ? AND collection_group = ? " @@ -188,21 +192,22 @@ public Map getOverlays( lastCollectionPath[0], lastDocumentPath[0], lastLargestBatchId[0]) - .forEach(row -> processOverlaysInBackground(backgroundQueue2, row)); - - backgroundQueue2.drainInto(results); - return results; + .forEach(row -> processOverlaysInBackground(backgroundQueue, result, row)); + backgroundQueue.drain(); + return result; } private void processOverlaysInBackground( - BackgroundQueue backgroundQueue, Cursor row) { + BackgroundQueue backgroundQueue, Map results, Cursor row) { byte[] rawMutation = row.getBlob(0); int largestBatchId = row.getInt(1); backgroundQueue.submit( - results -> { + () -> { Overlay overlay = decodeOverlay(rawMutation, largestBatchId); - results.put(overlay.getKey(), overlay); + synchronized (results) { + results.put(overlay.getKey(), overlay); + } }); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 7450b4a10c2..2c1a4dcdf88 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -168,19 +168,21 @@ public Map getAll(Iterable documentKe bindVars, ") ORDER BY path"); - BackgroundQueue backgroundQueue = new BackgroundQueue<>(); + BackgroundQueue backgroundQueue = new BackgroundQueue(); while (longQuery.hasMoreSubqueries()) { longQuery .performNextSubquery() - .forEach(row -> processRowInBackground(backgroundQueue, row, /*filter*/ null)); + .forEach(row -> processRowInBackground(backgroundQueue, results, row, /*filter*/ null)); } - - backgroundQueue.drainInto(results); + backgroundQueue.drain(); // Backfill any rows with null "document_type" discovered by processRowInBackground(). documentTypeBackfiller.backfill(db); - return results; + // Synchronize on `results` to avoid a data race with the background queue. + synchronized (results) { + return results; + } } @Override @@ -262,23 +264,26 @@ private Map getAll( } bindVars[i] = count; - BackgroundQueue backgroundQueue = new BackgroundQueue<>(); + BackgroundQueue backgroundQueue = new BackgroundQueue(); + Map results = new HashMap<>(); db.query(sql.toString()) .binding(bindVars) .forEach( row -> { - processRowInBackground(backgroundQueue, row, filter); + processRowInBackground(backgroundQueue, results, row, filter); if (context != null) { context.incrementDocumentReadCount(); } }); - - HashMap results = backgroundQueue.drain(); + backgroundQueue.drain(); // Backfill any null "document_type" columns discovered by processRowInBackground(). documentTypeBackfiller.backfill(db); - return results; + // Synchronize on `results` to avoid a data race with the background queue. + synchronized (results) { + return results; + } } private Map getAll( @@ -291,7 +296,8 @@ private Map getAll( } private void processRowInBackground( - BackgroundQueue backgroundQueue, + BackgroundQueue backgroundQueue, + Map results, Cursor row, @Nullable Function filter) { byte[] rawDocument = row.getBlob(0); @@ -301,14 +307,16 @@ private void processRowInBackground( String path = row.getString(4); backgroundQueue.submit( - results -> { + () -> { MutableDocument document = decodeMaybeDocument(rawDocument, readTimeSeconds, readTimeNanos); if (documentTypeIsNull) { documentTypeBackfiller.enqueue(path, readTimeSeconds, readTimeNanos, document); } if (filter == null || filter.apply(document)) { - results.put(document.getKey(), document); + synchronized (results) { + results.put(document.getKey(), document); + } } }); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt index 16cd0979d21..9e8cde8b6ca 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt @@ -27,29 +27,24 @@ import kotlinx.coroutines.asExecutor * called from the same thread. The behavior of an instance is undefined if any of the methods are * called from multiple threads. */ -internal class BackgroundQueue { +internal class BackgroundQueue { - private var state: State = State.Submitting() - - /** Overload for convenience of being called from Java code. */ - fun submit(consumer: Consumer>) = this.submit { consumer.accept(it) } + private var state: State = State.Submitting() /** * Submit a task for asynchronous execution on the executor of the owning [BackgroundQueue]. * * @throws IllegalStateException if [drain] has been called. */ - fun submit(runnable: (results: HashMap) -> Unit) { + fun submit(runnable: Runnable) { val submittingState = this.state - check(submittingState is State.Submitting) { - "submit() may not be called after drain() or drainInto()" - } + check(submittingState is State.Submitting) { "submit() may not be called after drain()" } submittingState.run { taskCount++ executor.execute { try { - runnable(threadLocalResults.get()!!) + runnable.run() } finally { completedTasks.release() } @@ -60,53 +55,22 @@ internal class BackgroundQueue { /** * Blocks until all tasks submitted via calls to [submit] have completed. * - * The results produced by each thread are merged into a new [HashMap] and returned. - * - * @throws IllegalStateException if [drain] or [drainInto] has already been called. - */ - fun drain(): HashMap = HashMap().also { drainInto(it) } - - /** - * Blocks until all tasks submitted via calls to [submit] have completed. - * - * The results produced by each thread are merged into the given map. - * - * @throws IllegalStateException if [drain] or [drainInto] has already been called. + * @throws IllegalStateException if called more than once. */ - fun drainInto(results: MutableMap) { + fun drain() { val submittingState = this.state - check(submittingState is State.Submitting) { "drain() or drainInto() has already been called" } - this.state = State.Draining() - return submittingState.run { - completedTasks.acquire(taskCount) - threadLocalResults.mergeResultsInto(results) - } - } - - private class ThreadLocalResults : ThreadLocal>() { - - private val results = mutableListOf>() + check(submittingState is State.Submitting) { "drain() may not be called more than once" } + this.state = State.Draining - override fun initialValue(): HashMap { - synchronized(results) { - val result = HashMap() - results.add(result) - return result - } - } - - fun mergeResultsInto(mergedResults: MutableMap) { - synchronized(results) { results.forEach { mergedResults.putAll(it) } } - } + submittingState.run { completedTasks.acquire(taskCount) } } - private sealed interface State { - class Submitting : State { + private sealed interface State { + class Submitting : State { val completedTasks = Semaphore(0) - val threadLocalResults = ThreadLocalResults() var taskCount: Int = 0 } - class Draining : State + object Draining : State } companion object { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java index 7189bf3747e..7580be68ad9 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java @@ -45,6 +45,7 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; +import java.util.Map; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -983,19 +984,25 @@ public void testSynchronousMatchesOrderBy() { // SQLiteRemoteDocumentCache.getAll(...), where `query.matches(doc)` is performed // for many different docs concurrently on the BackgroundQueue. Iterator iterator = docs.iterator(); - BackgroundQueue backgroundQueue = new BackgroundQueue<>(); + BackgroundQueue backgroundQueue = new BackgroundQueue(); + Map results = new HashMap<>(); while (iterator.hasNext()) { MutableDocument doc = iterator.next(); backgroundQueue.submit( - results -> { + () -> { // We call query.matches() to indirectly test query.matchesOrderBy() boolean result = query.matches(doc); - results.put(doc.getKey(), result); + + // We will include a synchronized block in our command to simulate + // the implementation in SQLiteRemoteDocumentCache.getAll(...) + synchronized (results) { + results.put(doc.getKey(), result); + } }); } - HashMap results = backgroundQueue.drain(); + backgroundQueue.drain(); Assert.assertEquals(101, results.keySet().size()); for (DocumentKey key : results.keySet()) { From 1b98cc5184db98d8c8ba4c26b3b491abf7868a26 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 15 Sep 2025 17:19:34 -0400 Subject: [PATCH 5/6] avoid asynchronous scheduling if there is only one row in the cursor --- .../firestore/local/SQLiteDocumentOverlayCache.java | 12 ++++++++++-- .../firestore/local/SQLiteRemoteDocumentCache.java | 12 ++++++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java index 53dc6415677..c7a07419fa9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java @@ -202,13 +202,21 @@ private void processOverlaysInBackground( byte[] rawMutation = row.getBlob(0); int largestBatchId = row.getInt(1); - backgroundQueue.submit( + Runnable runnable = () -> { Overlay overlay = decodeOverlay(rawMutation, largestBatchId); synchronized (results) { results.put(overlay.getKey(), overlay); } - }); + }; + + // If the cursor has exactly one row then just process that row synchronously to avoid the + // unnecessary overhead of scheduling its processing to run asynchronously. + if (row.isFirst() && row.isLast()) { + runnable.run(); + } else { + backgroundQueue.submit(runnable); + } } private Overlay decodeOverlay(byte[] rawMutation, int largestBatchId) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 2c1a4dcdf88..157992c2acd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -306,7 +306,7 @@ private void processRowInBackground( boolean documentTypeIsNull = row.isNull(3); String path = row.getString(4); - backgroundQueue.submit( + Runnable runnable = () -> { MutableDocument document = decodeMaybeDocument(rawDocument, readTimeSeconds, readTimeNanos); @@ -318,7 +318,15 @@ private void processRowInBackground( results.put(document.getKey(), document); } } - }); + }; + + // If the cursor has exactly one row then just process that row synchronously to avoid the + // unnecessary overhead of scheduling its processing to run asynchronously. + if (row.isFirst() && row.isLast()) { + runnable.run(); + } else { + backgroundQueue.submit(runnable); + } } @Override From a4b06c16f96331a0863eaa981d4700ddce5c11af Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 15 Sep 2025 22:22:46 -0400 Subject: [PATCH 6/6] Replace `submittingState.run` with directly accessing the properties, to improve readability. --- .../firebase/firestore/util/BackgroundQueue.kt | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt index 9e8cde8b6ca..14f38b6c260 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/BackgroundQueue.kt @@ -40,14 +40,12 @@ internal class BackgroundQueue { val submittingState = this.state check(submittingState is State.Submitting) { "submit() may not be called after drain()" } - submittingState.run { - taskCount++ - executor.execute { - try { - runnable.run() - } finally { - completedTasks.release() - } + submittingState.taskCount++ + executor.execute { + try { + runnable.run() + } finally { + submittingState.completedTasks.release() } } } @@ -62,7 +60,7 @@ internal class BackgroundQueue { check(submittingState is State.Submitting) { "drain() may not be called more than once" } this.state = State.Draining - submittingState.run { completedTasks.acquire(taskCount) } + submittingState.completedTasks.acquire(submittingState.taskCount) } private sealed interface State {