diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index d2c68fbc5cf..32a16b1c827 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -3,6 +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. # 26.0.0 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..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,13 +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)); + longQuery.performNextSubquery().forEach(row -> processRow(results, row, /*filter*/ null)); } - backgroundQueue.drain(); // Backfill any rows with null "document_type" discovered by processRowInBackground(). documentTypeBackfiller.backfill(db); @@ -266,18 +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); @@ -297,8 +288,7 @@ private Map getAll( collections, offset, count, /*tryFilterDocumentType*/ null, filter, /*context*/ null); } - private void processRowInBackground( - BackgroundQueue backgroundQueue, + private void processRow( Map results, Cursor row, @Nullable Function filter) { @@ -308,22 +298,15 @@ 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( - () -> { - 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); - } - } - }); + 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 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("__"); 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..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 @@ -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,10 +126,12 @@ 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(); + synchronized (lock) { + if (!overlayMap.isEmpty()) { + MapValue mergedResult = applyOverlayLocked(FieldPath.EMPTY_PATH, overlayMap); + if (mergedResult != null) { + partialValue = Value.newBuilder().setMapValue(mergedResult).build(); + } overlayMap.clear(); } } @@ -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;