diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index b11800c1e00..375c5d3d447 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -6,6 +6,8 @@ - [changed] Improve query performance in large result sets by replacing the deprecated AsyncTask thread pool with a self-managed thread pool. [#7376](//github.com/firebase/firebase-android-sdk/issues/7376) +- [changed] Improve query performance via internal memoization of calculated document data. + [#7370](//github.com/firebase/firebase-android-sdk/issues/7370) # 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 a7ea2997618..1620cf13da5 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 @@ -16,6 +16,7 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; +import androidx.annotation.GuardedBy; import androidx.annotation.NonNull; import androidx.annotation.Nullable; import com.google.firebase.firestore.model.mutation.FieldMask; @@ -28,10 +29,31 @@ /** A structured object value stored in Firestore. */ public final class ObjectValue implements Cloneable { + + private final Object lock = new Object(); + + /** + * The immutable Value proto for this object with all overlays applied. + *

+ * The value for this member is calculated _lazily_ and is null if the overlays have not yet been + * applied. + *

+ * This member MAY be READ concurrently from multiple threads without acquiring any particular + * locks; however, UPDATING it MUST have the `lock` lock held. + *

+ * Internal Invariant: Exactly one of `mergedValue` and `partialValue` must be null, with the + * other being non-null. + */ + @Nullable private volatile Value mergedValue; + /** * The immutable Value proto for this object. Local mutations are stored in `overlayMap` and only * applied when {@link #buildProto()} is invoked. + *

+ * Internal Invariant: Exactly one of `mergedValue` and `partialValue` must be null, with the + * other being non-null. */ + @GuardedBy("lock") private Value partialValue; /** @@ -39,6 +61,7 @@ public final class ObjectValue implements Cloneable { * #partialValue}. Values can either be {@link Value} protos, {@code Map} values * (to represent additional nesting) or {@code null} (to represent field deletes). */ + @GuardedBy("lock") private final Map overlayMap = new HashMap<>(); public static ObjectValue fromMap(Map value) { @@ -53,7 +76,7 @@ public ObjectValue(Value value) { hardAssert( !ServerTimestamps.isServerTimestamp(value), "ServerTimestamps should not be used as an ObjectValue"); - this.partialValue = value; + this.mergedValue = value; } public ObjectValue() { @@ -103,7 +126,7 @@ private FieldMask extractFieldMask(MapValue value) { } @Nullable - private Value extractNestedValue(Value value, FieldPath fieldPath) { + private static Value extractNestedValue(Value value, FieldPath fieldPath) { if (fieldPath.isEmpty()) { return value; } else { @@ -121,17 +144,40 @@ private Value extractNestedValue(Value value, FieldPath fieldPath) { * Returns the Protobuf that backs this ObjectValue. * *

This method applies any outstanding modifications and memoizes the result. Further - * invocations are based on this memoized result. + * invocations are based on this memoized result until overlays are applied, at which point the + * memoized result is marked as "stale" and a new result is calculated and memoized upon the next + * invocation of this method. */ private Value buildProto() { - synchronized (overlayMap) { - MapValue mergedResult = applyOverlay(FieldPath.EMPTY_PATH, overlayMap); - if (mergedResult != null) { - partialValue = Value.newBuilder().setMapValue(mergedResult).build(); - overlayMap.clear(); + // Use double-checked locking to avoid acquiring a lock in the cases where the memoized result + // has already been calculated (https://en.wikipedia.org/wiki/Double-checked_locking). + Value value = this.mergedValue; + + if (value == null) { + synchronized (lock) { + value = mergedValue; + if (value == null) { + assert (partialValue != null); + if (overlayMap.isEmpty()) { + value = partialValue; + } else { + MapValue mergedResult = applyOverlay(partialValue, FieldPath.EMPTY_PATH, overlayMap); + if (mergedResult == null) { + value = partialValue; + } else { + value = Value.newBuilder().setMapValue(mergedResult).build(); + } + } + + assert (value != null); + mergedValue = value; + partialValue = null; + overlayMap.clear(); + } } } - return partialValue; + + return value; } /** @@ -171,6 +217,17 @@ 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) { + synchronized (lock) { + if (mergedValue != null) { + partialValue = mergedValue; + mergedValue = null; + } + setOverlay(overlayMap, path, value); + } + } + + private static void setOverlay( + Map overlayMap, FieldPath path, @Nullable Value value) { Map currentLevel = overlayMap; for (int i = 0; i < path.length() - 1; ++i) { @@ -208,8 +265,8 @@ 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( - FieldPath currentPath, Map currentOverlays) { + private static @Nullable MapValue applyOverlay( + Value partialValue, FieldPath currentPath, Map currentOverlays) { boolean modified = false; @Nullable Value existingValue = extractNestedValue(partialValue, currentPath); @@ -227,7 +284,8 @@ private void setOverlay(FieldPath path, @Nullable Value value) { if (value instanceof Map) { @Nullable MapValue nested = - applyOverlay(currentPath.append(pathSegment), (Map) value); + applyOverlay( + partialValue, currentPath.append(pathSegment), (Map) value); if (nested != null) { resultAtPath.putFields(pathSegment, Value.newBuilder().setMapValue(nested).build()); modified = true;