From 9b76285b3860db921b54d8a9d072eae45a1dfba0 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 12 Sep 2025 13:13:30 -0600 Subject: [PATCH 1/7] Update ObjectValue.buildProto to return the memoized proto if there are no overlay changes. Also addressed race condition due to missing lock. --- .../firebase/firestore/model/ObjectValue.java | 62 ++++++++++--------- 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java index a7ea2997618..91017b50c30 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java @@ -28,6 +28,8 @@ /** A structured object value stored in Firestore. */ public final class ObjectValue implements Cloneable { + private final Object lock = new Object(); // Monitor object + /** * The immutable Value proto for this object. Local mutations are stored in `overlayMap` and only * applied when {@link #buildProto()} is invoked. @@ -124,11 +126,13 @@ private Value extractNestedValue(Value value, FieldPath fieldPath) { * invocations are based on this memoized result. */ private Value buildProto() { - synchronized (overlayMap) { - MapValue mergedResult = applyOverlay(FieldPath.EMPTY_PATH, overlayMap); - if (mergedResult != null) { - partialValue = Value.newBuilder().setMapValue(mergedResult).build(); - overlayMap.clear(); + synchronized (lock) { + if (!overlayMap.isEmpty()) { + MapValue mergedResult = applyOverlayLocked(FieldPath.EMPTY_PATH, overlayMap); + if (mergedResult != null) { + partialValue = Value.newBuilder().setMapValue(mergedResult).build(); + overlayMap.clear(); + } } } return partialValue; @@ -171,31 +175,33 @@ public void setAll(Map data) { * Adds {@code value} to the overlay map at {@code path}. Creates nested map entries if needed. */ private void setOverlay(FieldPath path, @Nullable Value value) { - Map currentLevel = overlayMap; + synchronized (lock) { + Map currentLevel = overlayMap; - for (int i = 0; i < path.length() - 1; ++i) { - String currentSegment = path.getSegment(i); - Object currentValue = currentLevel.get(currentSegment); + for (int i = 0; i < path.length() - 1; ++i) { + String currentSegment = path.getSegment(i); + Object currentValue = currentLevel.get(currentSegment); - if (currentValue instanceof Map) { - // Re-use a previously created map - currentLevel = (Map) currentValue; - } else if (currentValue instanceof Value - && ((Value) currentValue).getValueTypeCase() == Value.ValueTypeCase.MAP_VALUE) { - // Convert the existing Protobuf MapValue into a Java map - Map nextLevel = - new HashMap<>(((Value) currentValue).getMapValue().getFieldsMap()); - currentLevel.put(currentSegment, nextLevel); - currentLevel = nextLevel; - } else { - // Create an empty hash map to represent the current nesting level - Map nextLevel = new HashMap<>(); - currentLevel.put(currentSegment, nextLevel); - currentLevel = nextLevel; + if (currentValue instanceof Map) { + // Re-use a previously created map + currentLevel = (Map) currentValue; + } else if (currentValue instanceof Value + && ((Value) currentValue).getValueTypeCase() == Value.ValueTypeCase.MAP_VALUE) { + // Convert the existing Protobuf MapValue into a Java map + Map nextLevel = + new HashMap<>(((Value) currentValue).getMapValue().getFieldsMap()); + currentLevel.put(currentSegment, nextLevel); + currentLevel = nextLevel; + } else { + // Create an empty hash map to represent the current nesting level + Map nextLevel = new HashMap<>(); + currentLevel.put(currentSegment, nextLevel); + currentLevel = nextLevel; + } } - } - currentLevel.put(path.getLastSegment(), value); + currentLevel.put(path.getLastSegment(), value); + } } /** @@ -208,7 +214,7 @@ private void setOverlay(FieldPath path, @Nullable Value value) { * overlayMap}. * @return The merged data at `currentPath` or null if no modifications were applied. */ - private @Nullable MapValue applyOverlay( + private @Nullable MapValue applyOverlayLocked( FieldPath currentPath, Map currentOverlays) { boolean modified = false; @@ -227,7 +233,7 @@ private void setOverlay(FieldPath path, @Nullable Value value) { if (value instanceof Map) { @Nullable MapValue nested = - applyOverlay(currentPath.append(pathSegment), (Map) value); + applyOverlayLocked(currentPath.append(pathSegment), (Map) value); if (nested != null) { resultAtPath.putFields(pathSegment, Value.newBuilder().setMapValue(nested).build()); modified = true; From 077fb1c8b9d7ab168aed1743931899d3f0eebd40 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 12 Sep 2025 13:18:11 -0600 Subject: [PATCH 2/7] Update firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- .../java/com/google/firebase/firestore/model/ObjectValue.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java index 91017b50c30..cb8e601e46d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java @@ -131,8 +131,8 @@ private Value buildProto() { MapValue mergedResult = applyOverlayLocked(FieldPath.EMPTY_PATH, overlayMap); if (mergedResult != null) { partialValue = Value.newBuilder().setMapValue(mergedResult).build(); - overlayMap.clear(); } + overlayMap.clear(); } } return partialValue; From a56fb2efef0f102b7336d0bd7b9ad888bc4a637f Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 12 Sep 2025 13:26:00 -0600 Subject: [PATCH 3/7] Update CHANGELOG.md --- firebase-firestore/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index d2c68fbc5cf..b7cadb611b4 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -3,6 +3,7 @@ - [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 the performance of queries against SDK cache through internal memoization of document data. # 26.0.0 From db02504bc12fed3a2acd242617f5c54544634878 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 12 Sep 2025 13:28:35 -0600 Subject: [PATCH 4/7] formatting --- firebase-firestore/CHANGELOG.md | 3 ++- .../java/com/google/firebase/firestore/model/ObjectValue.java | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index b7cadb611b4..32a16b1c827 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -3,7 +3,8 @@ - [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 the performance of queries against SDK cache through internal memoization of document data. +- [changed] Improve the performance of queries against SDK cache through internal memoization of + document data. # 26.0.0 diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java index cb8e601e46d..9241f2a0212 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java @@ -186,10 +186,10 @@ private void setOverlay(FieldPath path, @Nullable Value value) { // Re-use a previously created map currentLevel = (Map) currentValue; } else if (currentValue instanceof Value - && ((Value) currentValue).getValueTypeCase() == Value.ValueTypeCase.MAP_VALUE) { + && ((Value) currentValue).getValueTypeCase() == Value.ValueTypeCase.MAP_VALUE) { // Convert the existing Protobuf MapValue into a Java map Map nextLevel = - new HashMap<>(((Value) currentValue).getMapValue().getFieldsMap()); + new HashMap<>(((Value) currentValue).getMapValue().getFieldsMap()); currentLevel.put(currentSegment, nextLevel); currentLevel = nextLevel; } else { From 3034608b775e2267c9c0e673f9b5a1652f93f9b4 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Mon, 15 Sep 2025 09:17:25 -0600 Subject: [PATCH 5/7] Speed improvements in BasePath by converting the path to utf8 segments one time, and then always comparing on utf8 arrays --- .../firebase/firestore/model/BasePath.java | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java index bc630f94475..9f0deffccf4 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java @@ -18,6 +18,7 @@ import androidx.annotation.NonNull; import com.google.firebase.firestore.util.Util; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; @@ -27,6 +28,7 @@ */ public abstract class BasePath> implements Comparable { final List segments; + List utf8Segments = null; BasePath(List segments) { this.segments = segments; @@ -93,8 +95,12 @@ public int compareTo(@NonNull B o) { int i = 0; int myLength = length(); int theirLength = o.length(); + List myUtf8Segments = ensureUtf8Segments(); + List theirUtf8Segments = o.ensureUtf8Segments(); while (i < myLength && i < theirLength) { - int localCompare = compareSegments(getSegment(i), o.getSegment(i)); + int localCompare = + compareSegments( + myUtf8Segments.get(i), theirUtf8Segments.get(i), getSegment(i), o.getSegment(i)); if (localCompare != 0) { return localCompare; } @@ -103,21 +109,39 @@ public int compareTo(@NonNull B o) { return Integer.compare(myLength, theirLength); } - private static int compareSegments(String lhs, String rhs) { - boolean isLhsNumeric = isNumericId(lhs); - boolean isRhsNumeric = isNumericId(rhs); + private static int compareSegments(byte[] lhs, byte[] rhs, String lhsString, String rhsString) { + boolean isLhsNumeric = lhs == null; + boolean isRhsNumeric = rhs == null; if (isLhsNumeric && !isRhsNumeric) { // Only lhs is numeric return -1; } else if (!isLhsNumeric && isRhsNumeric) { // Only rhs is numeric return 1; } else if (isLhsNumeric && isRhsNumeric) { // both numeric - return Long.compare(extractNumericId(lhs), extractNumericId(rhs)); + return Long.compare(extractNumericId(lhsString), extractNumericId(rhsString)); } else { // both string - return Util.compareUtf8Strings(lhs, rhs); + return Util.compareByteArrays(lhs, rhs); } } + public List ensureUtf8Segments() { + if (this.utf8Segments == null) { + this.utf8Segments = new ArrayList<>(this.segments.size()); + for (int i = 0; i < this.segments.size(); i++) { + String segment = this.segments.get(i); + boolean isNumeric = isNumericId(segment); + if (!isNumeric) { + byte[] utf8Bytes = segment.getBytes(StandardCharsets.UTF_8); + this.utf8Segments.add(utf8Bytes); + } else { + this.utf8Segments.add(null); + } + } + } + + return this.utf8Segments; + } + /** Checks if a segment is a numeric ID (starts with "__id" and ends with "__"). */ private static boolean isNumericId(String segment) { return segment.startsWith("__id") && segment.endsWith("__"); From 297b5a9d5538cb386db16dba541aea502d9987ef Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Mon, 15 Sep 2025 09:20:37 -0600 Subject: [PATCH 6/7] WIP: Move processing of rows from a background thread to the main thread. Reducing the overhead of queueing each row for processing on a background thread. --- .../local/SQLiteRemoteDocumentCache.java | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) 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..0eac705eaa2 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 @@ -170,13 +170,15 @@ 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, results, row, /*filter*/ + // null)); + .forEach(row -> processRow(results, row, /*filter*/ null)); } - backgroundQueue.drain(); + // backgroundQueue.drain(); // Backfill any rows with null "document_type" discovered by processRowInBackground(). documentTypeBackfiller.backfill(db); @@ -266,18 +268,19 @@ 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, results, row, filter); + // processRowInBackground(backgroundQueue, results, row, filter); + processRow(results, row, filter); if (context != null) { context.incrementDocumentReadCount(); } }); - backgroundQueue.drain(); + // backgroundQueue.drain(); // Backfill any null "document_type" columns discovered by processRowInBackground(). documentTypeBackfiller.backfill(db); @@ -326,6 +329,27 @@ private void processRowInBackground( }); } + private void processRow( + Map results, + Cursor row, + @Nullable Function filter) { + byte[] rawDocument = row.getBlob(0); + int readTimeSeconds = row.getInt(1); + int readTimeNanos = row.getInt(2); + boolean documentTypeIsNull = row.isNull(3); + String path = row.getString(4); + + 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); + } + } + } + @Override public Map getDocumentsMatchingQuery( Query query, IndexOffset offset, @Nonnull Set mutatedKeys) { From cca64069ad33f4ff62c915ca845eb0c8c5651b2f Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Mon, 15 Sep 2025 09:24:13 -0600 Subject: [PATCH 7/7] Remove unused or commented code --- .../local/SQLiteRemoteDocumentCache.java | 43 +------------------ 1 file changed, 1 insertion(+), 42 deletions(-) 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 0eac705eaa2..ce4db23be94 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 @@ -32,8 +32,6 @@ import com.google.firebase.firestore.model.MutableDocument; 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 +45,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; @@ -170,15 +167,9 @@ public Map getAll(Iterable documentKe bindVars, ") ORDER BY path"); - // BackgroundQueue backgroundQueue = new BackgroundQueue(); while (longQuery.hasMoreSubqueries()) { - longQuery - .performNextSubquery() - // .forEach(row -> processRowInBackground(backgroundQueue, results, row, /*filter*/ - // null)); - .forEach(row -> processRow(results, row, /*filter*/ null)); + longQuery.performNextSubquery().forEach(row -> processRow(results, row, /*filter*/ null)); } - // backgroundQueue.drain(); // Backfill any rows with null "document_type" discovered by processRowInBackground(). documentTypeBackfiller.backfill(db); @@ -268,19 +259,16 @@ private Map getAll( } bindVars[i] = count; - // BackgroundQueue backgroundQueue = new BackgroundQueue(); Map results = new HashMap<>(); db.query(sql.toString()) .binding(bindVars) .forEach( row -> { - // processRowInBackground(backgroundQueue, results, row, filter); processRow(results, row, filter); if (context != null) { context.incrementDocumentReadCount(); } }); - // backgroundQueue.drain(); // Backfill any null "document_type" columns discovered by processRowInBackground(). documentTypeBackfiller.backfill(db); @@ -300,35 +288,6 @@ private Map getAll( collections, offset, count, /*tryFilterDocumentType*/ null, filter, /*context*/ null); } - private void processRowInBackground( - BackgroundQueue backgroundQueue, - Map results, - Cursor row, - @Nullable Function filter) { - byte[] rawDocument = row.getBlob(0); - int readTimeSeconds = row.getInt(1); - int readTimeNanos = row.getInt(2); - 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( - () -> { - 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); - } - } - }); - } - private void processRow( Map results, Cursor row,