Skip to content

Commit c11a7a4

Browse files
schmidt-sebastianschmidt-sebastian
authored
Performance: Don't deserialize full document Proto for Query execution (#561)
1 parent a478b26 commit c11a7a4

File tree

17 files changed

+135
-73
lines changed

17 files changed

+135
-73
lines changed

‎firebase-firestore/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
# Unreleased
2+
- [changed] Improved performance for queries with filters that only return a
3+
small subset of the documents in a collection.
24
- [changed] Instead of failing silently, Firestore now crashes the client app
35
if it fails to load SSL Ciphers. To avoid these crashes, you must bundle
46
Conscrypt to support non-GMSCore devices on Android KitKat or JellyBean (see

‎firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/testutil/TestUtil.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -175,21 +175,21 @@ public static SnapshotVersion version(long versionMicros) {
175175

176176
public static Document doc(String key, long version, Map<String, Object> data) {
177177
return new Document(
178-
key(key), version(version), wrapObject(data), Document.DocumentState.SYNCED);
178+
key(key), version(version), Document.DocumentState.SYNCED, wrapObject(data));
179179
}
180180

181181
public static Document doc(DocumentKey key, long version, Map<String, Object> data) {
182-
return new Document(key, version(version), wrapObject(data), Document.DocumentState.SYNCED);
182+
return new Document(key, version(version), Document.DocumentState.SYNCED, wrapObject(data));
183183
}
184184

185185
public static Document doc(
186186
String key, long version, ObjectValue data, Document.DocumentState documentState) {
187-
return new Document(key(key), version(version), data, documentState);
187+
return new Document(key(key), version(version), documentState, data);
188188
}
189189

190190
public static Document doc(
191191
String key, long version, Map<String, Object> data, Document.DocumentState documentState) {
192-
return new Document(key(key), version(version), wrapObject(data), documentState);
192+
return new Document(key(key), version(version), documentState, wrapObject(data));
193193
}
194194

195195
public static NoDocument deletedDoc(String key, long version) {

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,15 +112,15 @@ private com.google.firestore.v1.Document encodeDocument(Document document) {
112112
private Document decodeDocument(
113113
com.google.firestore.v1.Document document, boolean hasCommittedMutations) {
114114
DocumentKey key = rpcSerializer.decodeKey(document.getName());
115-
ObjectValue value = rpcSerializer.decodeFields(document.getFieldsMap());
116115
SnapshotVersion version = rpcSerializer.decodeVersion(document.getUpdateTime());
117116
return new Document(
118117
key,
119118
version,
120-
value,
121119
hasCommittedMutations
122120
? Document.DocumentState.COMMITTED_MUTATIONS
123-
: Document.DocumentState.SYNCED);
121+
: Document.DocumentState.SYNCED,
122+
document,
123+
rpcSerializer::decodeValue);
124124
}
125125

126126
/** Encodes a NoDocument value to the equivalent proto. */

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/Document.java

Lines changed: 84 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,16 @@
1414

1515
package com.google.firebase.firestore.model;
1616

17+
import static com.google.firebase.firestore.util.Assert.hardAssert;
18+
19+
import com.google.common.base.Function;
1720
import com.google.firebase.firestore.model.value.FieldValue;
1821
import com.google.firebase.firestore.model.value.ObjectValue;
22+
import com.google.firestore.v1.Value;
1923
import java.util.Comparator;
24+
import java.util.Map;
25+
import java.util.concurrent.ConcurrentHashMap;
26+
import javax.annotation.Nonnull;
2027
import javax.annotation.Nullable;
2128

2229
/**
@@ -36,58 +43,107 @@ public enum DocumentState {
3643
}
3744

3845
private static final Comparator<Document> KEY_COMPARATOR =
39-
new Comparator<Document>() {
40-
@Override
41-
public int compare(Document left, Document right) {
42-
return left.getKey().compareTo(right.getKey());
43-
}
44-
};
46+
(left, right) -> left.getKey().compareTo(right.getKey());
4547

4648
/** A document comparator that returns document by key and key only. */
4749
public static Comparator<Document> keyComparator() {
4850
return KEY_COMPARATOR;
4951
}
5052

51-
private final ObjectValue data;
52-
5353
private final DocumentState documentState;
54+
private @Nullable final com.google.firestore.v1.Document proto;
55+
private @Nullable final Function<Value, FieldValue> converter;
56+
private @Nullable ObjectValue objectValue;
5457

55-
/**
56-
* Memoized serialized form of the document for optimization purposes (avoids repeated
57-
* serialization). Might be null.
58-
*/
59-
private final com.google.firestore.v1.Document proto;
60-
61-
public @Nullable com.google.firestore.v1.Document getProto() {
62-
return proto;
63-
}
58+
/** A cache for FieldValues that have already been deserialized in `getField()`. */
59+
private @Nullable Map<FieldPath, FieldValue> fieldValueCache;
6460

6561
public Document(
66-
DocumentKey key, SnapshotVersion version, ObjectValue data, DocumentState documentState) {
62+
DocumentKey key,
63+
SnapshotVersion version,
64+
DocumentState documentState,
65+
ObjectValue objectValue) {
6766
super(key, version);
68-
this.data = data;
6967
this.documentState = documentState;
68+
this.objectValue = objectValue;
7069
this.proto = null;
70+
this.converter = null;
7171
}
7272

7373
public Document(
7474
DocumentKey key,
7575
SnapshotVersion version,
76-
ObjectValue data,
7776
DocumentState documentState,
78-
com.google.firestore.v1.Document proto) {
77+
com.google.firestore.v1.Document proto,
78+
Function<com.google.firestore.v1.Value, FieldValue> converter) {
7979
super(key, version);
80-
this.data = data;
8180
this.documentState = documentState;
8281
this.proto = proto;
82+
this.converter = converter;
8383
}
8484

85+
/**
86+
* Memoized serialized form of the document for optimization purposes (avoids repeated
87+
* serialization). Might be null.
88+
*/
89+
public @Nullable com.google.firestore.v1.Document getProto() {
90+
return proto;
91+
}
92+
93+
@Nonnull
8594
public ObjectValue getData() {
86-
return data;
95+
if (objectValue == null) {
96+
hardAssert(proto != null && converter != null, "Expected proto and converter to be non-null");
97+
98+
ObjectValue result = ObjectValue.emptyObject();
99+
for (Map.Entry<String, com.google.firestore.v1.Value> entry :
100+
proto.getFieldsMap().entrySet()) {
101+
FieldPath path = FieldPath.fromSingleSegment(entry.getKey());
102+
FieldValue value = converter.apply(entry.getValue());
103+
result = result.set(path, value);
104+
}
105+
objectValue = result;
106+
107+
// Once objectValue is computed, values inside the fieldValueCache are no longer accessed.
108+
fieldValueCache = null;
109+
}
110+
111+
return objectValue;
87112
}
88113

89114
public @Nullable FieldValue getField(FieldPath path) {
90-
return data.get(path);
115+
if (objectValue != null) {
116+
return objectValue.get(path);
117+
} else {
118+
hardAssert(proto != null && converter != null, "Expected proto and converter to be non-null");
119+
120+
if (fieldValueCache == null) {
121+
// TODO(b/136090445): Remove the cache when `getField` is no longer called during Query
122+
// ordering.
123+
fieldValueCache = new ConcurrentHashMap<>();
124+
}
125+
126+
FieldValue fieldValue = fieldValueCache.get(path);
127+
if (fieldValue == null) {
128+
// Instead of deserializing the full Document proto, we only deserialize the value at
129+
// the requested field path. This speeds up Query execution as query filters can discard
130+
// documents based on a single field.
131+
Value protoValue = proto.getFieldsMap().get(path.getFirstSegment());
132+
for (int i = 1; protoValue != null && i < path.length(); ++i) {
133+
if (protoValue.getValueTypeCase() != Value.ValueTypeCase.MAP_VALUE) {
134+
return null;
135+
}
136+
protoValue = protoValue.getMapValue().getFieldsMap().get(path.getSegment(i));
137+
}
138+
139+
if (protoValue != null) {
140+
fieldValue = converter.apply(protoValue);
141+
fieldValueCache.put(path, fieldValue);
142+
}
143+
}
144+
145+
return fieldValue;
146+
}
91147
}
92148

93149
public @Nullable Object getFieldValue(FieldPath path) {
@@ -113,7 +169,7 @@ public boolean equals(Object o) {
113169
if (this == o) {
114170
return true;
115171
}
116-
if (o == null || getClass() != o.getClass()) {
172+
if (!(o instanceof Document)) {
117173
return false;
118174
}
119175

@@ -122,13 +178,13 @@ public boolean equals(Object o) {
122178
return getVersion().equals(document.getVersion())
123179
&& getKey().equals(document.getKey())
124180
&& documentState.equals(document.documentState)
125-
&& data.equals(document.data);
181+
&& getData().equals(document.getData());
126182
}
127183

128184
@Override
129185
public int hashCode() {
186+
// Note: We deliberately decided to omit `getData()` since its computation is expensive.
130187
int result = getKey().hashCode();
131-
result = 31 * result + data.hashCode();
132188
result = 31 * result + getVersion().hashCode();
133189
result = 31 * result + documentState.hashCode();
134190
return result;
@@ -140,7 +196,7 @@ public String toString() {
140196
+ "key="
141197
+ getKey()
142198
+ ", data="
143-
+ data
199+
+ getData()
144200
+ ", version="
145201
+ getVersion()
146202
+ ", documentState="

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/PatchMutation.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ public MaybeDocument applyToRemoteDocument(
112112

113113
SnapshotVersion version = mutationResult.getVersion();
114114
ObjectValue newData = patchDocument(maybeDoc);
115-
return new Document(getKey(), version, newData, Document.DocumentState.COMMITTED_MUTATIONS);
115+
return new Document(getKey(), version, Document.DocumentState.COMMITTED_MUTATIONS, newData);
116116
}
117117

118118
@Nullable
@@ -127,7 +127,7 @@ public MaybeDocument applyToLocalView(
127127

128128
SnapshotVersion version = getPostMutationVersion(maybeDoc);
129129
ObjectValue newData = patchDocument(maybeDoc);
130-
return new Document(getKey(), version, newData, Document.DocumentState.LOCAL_MUTATIONS);
130+
return new Document(getKey(), version, Document.DocumentState.LOCAL_MUTATIONS, newData);
131131
}
132132

133133
@Nullable

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/SetMutation.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public MaybeDocument applyToRemoteDocument(
7272
// accepted the mutation so the precondition must have held.
7373

7474
SnapshotVersion version = mutationResult.getVersion();
75-
return new Document(getKey(), version, value, Document.DocumentState.COMMITTED_MUTATIONS);
75+
return new Document(getKey(), version, Document.DocumentState.COMMITTED_MUTATIONS, value);
7676
}
7777

7878
@Nullable
@@ -86,7 +86,7 @@ public MaybeDocument applyToLocalView(
8686
}
8787

8888
SnapshotVersion version = getPostMutationVersion(maybeDoc);
89-
return new Document(getKey(), version, value, Document.DocumentState.LOCAL_MUTATIONS);
89+
return new Document(getKey(), version, Document.DocumentState.LOCAL_MUTATIONS, value);
9090
}
9191

9292
@Nullable

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformMutation.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ public MaybeDocument applyToRemoteDocument(
103103
serverTransformResults(doc, mutationResult.getTransformResults());
104104
ObjectValue newData = transformObject(doc.getData(), transformResults);
105105
return new Document(
106-
getKey(), mutationResult.getVersion(), newData, Document.DocumentState.COMMITTED_MUTATIONS);
106+
getKey(), mutationResult.getVersion(), Document.DocumentState.COMMITTED_MUTATIONS, newData);
107107
}
108108

109109
@Nullable
@@ -120,7 +120,7 @@ public MaybeDocument applyToLocalView(
120120
List<FieldValue> transformResults = localTransformResults(localWriteTime, baseDoc);
121121
ObjectValue newData = transformObject(doc.getData(), transformResults);
122122
return new Document(
123-
getKey(), doc.getVersion(), newData, Document.DocumentState.LOCAL_MUTATIONS);
123+
getKey(), doc.getVersion(), Document.DocumentState.LOCAL_MUTATIONS, newData);
124124
}
125125

126126
@Override

‎firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -407,11 +407,11 @@ private Document decodeFoundDocument(BatchGetDocumentsResponse response) {
407407
response.getResultCase().equals(ResultCase.FOUND),
408408
"Tried to deserialize a found document from a missing document.");
409409
DocumentKey key = decodeKey(response.getFound().getName());
410-
ObjectValue value = decodeFields(response.getFound().getFieldsMap());
411410
SnapshotVersion version = decodeVersion(response.getFound().getUpdateTime());
412411
hardAssert(
413412
!version.equals(SnapshotVersion.NONE), "Got a document response with no snapshot version");
414-
return new Document(key, version, value, Document.DocumentState.SYNCED, response.getFound());
413+
return new Document(
414+
key, version, Document.DocumentState.SYNCED, response.getFound(), this::decodeValue);
415415
}
416416

417417
private NoDocument decodeMissingDocument(BatchGetDocumentsResponse response) {
@@ -1055,12 +1055,13 @@ public WatchChange decodeWatchChange(ListenResponse protoChange) {
10551055
SnapshotVersion version = decodeVersion(docChange.getDocument().getUpdateTime());
10561056
hardAssert(
10571057
!version.equals(SnapshotVersion.NONE), "Got a document change without an update time");
1058-
ObjectValue data = decodeFields(docChange.getDocument().getFieldsMap());
1059-
// The document may soon be re-serialized back to protos in order to store it in local
1060-
// persistence. Memoize the encoded form to avoid encoding it again.
10611058
Document document =
10621059
new Document(
1063-
key, version, data, Document.DocumentState.SYNCED, docChange.getDocument());
1060+
key,
1061+
version,
1062+
Document.DocumentState.SYNCED,
1063+
docChange.getDocument(),
1064+
this::decodeValue);
10641065
watchChange = new WatchChange.DocumentChange(added, removed, document.getKey(), document);
10651066
break;
10661067
case DOCUMENT_DELETE:

‎firebase-firestore/src/test/java/com/google/firebase/firestore/DocumentSnapshotTest.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,14 @@ public void testEquals() {
4545
assertNotEquals(base, differentData);
4646
assertNotEquals(base, fromCache);
4747

48+
// The assertions below that hash codes of different values are not equal is not something that
49+
// we guarantee. In particular `base` and `differentData` have a hash collision because we
50+
// don't use data in the hashCode.
4851
assertEquals(base.hashCode(), baseDup.hashCode());
4952
assertEquals(noData.hashCode(), noDataDup.hashCode());
5053
assertNotEquals(base.hashCode(), noData.hashCode());
5154
assertNotEquals(noData.hashCode(), base.hashCode());
5255
assertNotEquals(base.hashCode(), differentPath.hashCode());
53-
assertNotEquals(base.hashCode(), differentData.hashCode());
5456
assertNotEquals(base.hashCode(), fromCache.hashCode());
5557
}
5658
}

‎firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,10 @@ public void testEquals() {
7171
assertNotEquals(foo, noPendingWrites);
7272
assertNotEquals(foo, fromCache);
7373

74+
// Note: `foo` and `differentDoc` have the same hash code since we no longer take document
75+
// contents into account.
7476
assertEquals(foo.hashCode(), fooDup.hashCode());
7577
assertNotEquals(foo.hashCode(), differentPath.hashCode());
76-
assertNotEquals(foo.hashCode(), differentDoc.hashCode());
7778
assertNotEquals(foo.hashCode(), noPendingWrites.hashCode());
7879
assertNotEquals(foo.hashCode(), fromCache.hashCode());
7980
}

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -229,15 +229,15 @@ public void testHandlesSetMutation() {
229229
assertChanged(doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.LOCAL_MUTATIONS));
230230
assertContains(doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.LOCAL_MUTATIONS));
231231

232-
acknowledgeMutation(0);
233-
assertChanged(doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.COMMITTED_MUTATIONS));
232+
acknowledgeMutation(1);
233+
assertChanged(doc("foo/bar", 1, map("foo", "bar"), Document.DocumentState.COMMITTED_MUTATIONS));
234234
if (garbageCollectorIsEager()) {
235235
// Nothing is pinning this anymore, as it has been acknowledged and there are no targets
236236
// active.
237237
assertNotContains("foo/bar");
238238
} else {
239239
assertContains(
240-
doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.COMMITTED_MUTATIONS));
240+
doc("foo/bar", 1, map("foo", "bar"), Document.DocumentState.COMMITTED_MUTATIONS));
241241
}
242242
}
243243

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -553,7 +553,7 @@ public void testRemoveTargetsThenGC() {
553553
() -> {
554554
SnapshotVersion newVersion = version(3);
555555
Document doc =
556-
new Document(middleDocToUpdate, newVersion, testValue, Document.DocumentState.SYNCED);
556+
new Document(middleDocToUpdate, newVersion, Document.DocumentState.SYNCED, testValue);
557557
documentCache.add(doc);
558558
updateTargetInTransaction(middleTarget);
559559
});

‎firebase-firestore/src/test/java/com/google/firebase/firestore/model/DocumentTest.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,10 @@
3636
public class DocumentTest {
3737

3838
@Test
39-
public void testConstructor() {
39+
public void testInstantiation() {
4040
Document document =
4141
new Document(
42-
key("messages/first"), version(1), wrapObject("a", 1), Document.DocumentState.SYNCED);
42+
key("messages/first"), version(1), Document.DocumentState.SYNCED, wrapObject("a", 1));
4343

4444
assertEquals(key("messages/first"), document.getKey());
4545
assertEquals(version(1), document.getVersion());
@@ -56,7 +56,7 @@ public void testExtractFields() {
5656
"owner",
5757
map("name", "Jonny", "title", "scallywag"));
5858
Document document =
59-
new Document(key("rooms/eros"), version(1), data, Document.DocumentState.SYNCED);
59+
new Document(key("rooms/eros"), version(1), Document.DocumentState.SYNCED, data);
6060

6161
assertEquals("Discuss all the project related stuff", document.getFieldValue(field("desc")));
6262
assertEquals("scallywag", document.getFieldValue(field("owner.title")));

0 commit comments

Comments
 (0)