Skip to content

Commit 9f6ed13

Browse files
var-constvar-const
authored
Port performance optimizations to speed up reading large collections from Android (#2140)
Straightforward port of firebase/firebase-android-sdk#123.
1 parent 673b085 commit 9f6ed13

12 files changed

+196
-19
lines changed

‎Firestore/Example/Tests/Local/FSTRemoteDocumentCacheTests.mm

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,37 @@ - (void)testSetAndReadADocument {
8181
[self setAndReadADocumentAtPath:kDocPath];
8282
}
8383

84+
- (void)testSetAndReadSeveralDocuments {
85+
if (!self.remoteDocumentCache) return;
86+
87+
self.persistence.run("testSetAndReadSeveralDocuments", [=]() {
88+
NSArray<FSTDocument *> *written =
89+
@[ [self setTestDocumentAtPath:kDocPath], [self setTestDocumentAtPath:kLongDocPath] ];
90+
MaybeDocumentMap read = [self.remoteDocumentCache
91+
entriesForKeys:DocumentKeySet{testutil::Key(kDocPath), testutil::Key(kLongDocPath)}];
92+
[self expectMap:read hasDocsInArray:written exactly:YES];
93+
});
94+
}
95+
96+
- (void)testSetAndReadSeveralDocumentsIncludingMissingDocument {
97+
if (!self.remoteDocumentCache) return;
98+
99+
self.persistence.run("testSetAndReadSeveralDocumentsIncludingMissingDocument", [=]() {
100+
NSArray<FSTDocument *> *written =
101+
@[ [self setTestDocumentAtPath:kDocPath], [self setTestDocumentAtPath:kLongDocPath] ];
102+
MaybeDocumentMap read =
103+
[self.remoteDocumentCache entriesForKeys:DocumentKeySet{
104+
testutil::Key(kDocPath),
105+
testutil::Key(kLongDocPath),
106+
testutil::Key("foo/nonexistent"),
107+
}];
108+
[self expectMap:read hasDocsInArray:written exactly:NO];
109+
auto found = read.find(DocumentKey::FromPathString("foo/nonexistent"));
110+
XCTAssertTrue(found != read.end());
111+
XCTAssertNil(found->second);
112+
});
113+
}
114+
84115
- (void)testSetAndReadADocumentAtDeepPath {
85116
if (!self.remoteDocumentCache) return;
86117

@@ -144,7 +175,7 @@ - (void)testDocumentsMatchingQuery {
144175

145176
FSTQuery *query = FSTTestQuery("b");
146177
DocumentMap results = [self.remoteDocumentCache documentsMatchingQuery:query];
147-
[self expectMap:results
178+
[self expectMap:results.underlying_map()
148179
hasDocsInArray:@[
149180
FSTTestDoc("b/1", kVersion, _kDocData, FSTDocumentStateSynced),
150181
FSTTestDoc("b/2", kVersion, _kDocData, FSTDocumentStateSynced)
@@ -162,16 +193,16 @@ - (FSTDocument *)setTestDocumentAtPath:(const absl::string_view)path {
162193
return doc;
163194
}
164195

165-
- (void)expectMap:(const DocumentMap &)map
196+
- (void)expectMap:(const MaybeDocumentMap &)map
166197
hasDocsInArray:(NSArray<FSTDocument *> *)expected
167198
exactly:(BOOL)exactly {
168199
if (exactly) {
169200
XCTAssertEqual(map.size(), [expected count]);
170201
}
171202
for (FSTDocument *doc in expected) {
172203
FSTDocument *actual = nil;
173-
auto found = map.underlying_map().find(doc.key);
174-
if (found != map.underlying_map().end()) {
204+
auto found = map.find(doc.key);
205+
if (found != map.end()) {
175206
actual = static_cast<FSTDocument *>(found->second);
176207
}
177208
XCTAssertEqualObjects(actual, doc);

‎Firestore/Source/Local/FSTLevelDBRemoteDocumentCache.mm

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,24 @@ - (nullable FSTMaybeDocument *)entryForKey:(const DocumentKey &)documentKey {
8585
}
8686
}
8787

88+
- (MaybeDocumentMap)entriesForKeys:(const DocumentKeySet &)keys {
89+
MaybeDocumentMap results;
90+
91+
LevelDbRemoteDocumentKey currentKey;
92+
auto it = _db.currentTransaction->NewIterator();
93+
94+
for (const DocumentKey &key : keys) {
95+
it->Seek([self remoteDocumentKey:key]);
96+
if (!it->Valid() || !currentKey.Decode(it->key()) || currentKey.document_key() != key) {
97+
results = results.insert(key, nil);
98+
} else {
99+
results = results.insert(key, [self decodeMaybeDocument:it->value() withKey:key]);
100+
}
101+
}
102+
103+
return results;
104+
}
105+
88106
- (DocumentMap)documentsMatchingQuery:(FSTQuery *)query {
89107
DocumentMap results;
90108

‎Firestore/Source/Local/FSTLocalDocumentsView.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,13 @@ NS_ASSUME_NONNULL_BEGIN
5555
- (firebase::firestore::model::MaybeDocumentMap)documentsForKeys:
5656
(const firebase::firestore::model::DocumentKeySet &)keys;
5757

58+
/**
59+
* Similar to `documentsForKeys`, but creates the local view from the given
60+
* `baseDocs` without retrieving documents from the local store.
61+
*/
62+
- (firebase::firestore::model::MaybeDocumentMap)localViewsForDocuments:
63+
(const firebase::firestore::model::MaybeDocumentMap &)baseDocs;
64+
5865
/** Performs a query against the local view of all documents. */
5966
- (firebase::firestore::model::DocumentMap)documentsMatchingQuery:(FSTQuery *)query;
6067

‎Firestore/Source/Local/FSTLocalDocumentsView.mm

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,48 @@ - (nullable FSTMaybeDocument *)documentForKey:(const DocumentKey &)key
8080
return document;
8181
}
8282

83+
// Returns the view of the given `docs` as they would appear after applying all
84+
// mutations in the given `batches`.
85+
- (MaybeDocumentMap)applyLocalMutationsToDocuments:(const MaybeDocumentMap &)docs
86+
fromBatches:(NSArray<FSTMutationBatch *> *)batches {
87+
MaybeDocumentMap results;
88+
89+
for (const auto &kv : docs) {
90+
const DocumentKey &key = kv.first;
91+
FSTMaybeDocument *localView = kv.second;
92+
for (FSTMutationBatch *batch in batches) {
93+
localView = [batch applyToLocalDocument:localView documentKey:key];
94+
}
95+
results = results.insert(key, localView);
96+
}
97+
return results;
98+
}
99+
83100
- (MaybeDocumentMap)documentsForKeys:(const DocumentKeySet &)keys {
101+
MaybeDocumentMap docs = [self.remoteDocumentCache entriesForKeys:keys];
102+
return [self localViewsForDocuments:docs];
103+
}
104+
105+
/**
106+
* Similar to `documentsForKeys`, but creates the local view from the given
107+
* `baseDocs` without retrieving documents from the local store.
108+
*/
109+
- (MaybeDocumentMap)localViewsForDocuments:(const MaybeDocumentMap &)baseDocs {
84110
MaybeDocumentMap results;
111+
112+
DocumentKeySet allKeys;
113+
for (const auto &kv : baseDocs) {
114+
allKeys = allKeys.insert(kv.first);
115+
}
85116
NSArray<FSTMutationBatch *> *batches =
86-
[self.mutationQueue allMutationBatchesAffectingDocumentKeys:keys];
87-
for (const DocumentKey &key : keys) {
88-
// TODO(mikelehen): PERF: Consider fetching all remote documents at once rather than one-by-one.
89-
FSTMaybeDocument *maybeDoc = [self documentForKey:key inBatches:batches];
117+
[self.mutationQueue allMutationBatchesAffectingDocumentKeys:allKeys];
118+
119+
MaybeDocumentMap docs = [self applyLocalMutationsToDocuments:baseDocs fromBatches:batches];
120+
121+
for (const auto &kv : docs) {
122+
const DocumentKey &key = kv.first;
123+
FSTMaybeDocument *maybeDoc = kv.second;
124+
90125
// TODO(http://b/32275378): Don't conflate missing / deleted.
91126
if (!maybeDoc) {
92127
maybeDoc = [FSTDeletedDocument documentWithKey:key

‎Firestore/Source/Local/FSTLocalSerializer.mm

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,11 @@ - (FSTPBMaybeDocument *)encodedMaybeDocument:(FSTMaybeDocument *)document {
6767
proto.hasCommittedMutations = deletedDocument.hasCommittedMutations;
6868
} else if ([document isKindOfClass:[FSTDocument class]]) {
6969
FSTDocument *existingDocument = (FSTDocument *)document;
70-
proto.document = [self encodedDocument:existingDocument];
70+
if (existingDocument.proto != nil) {
71+
proto.document = existingDocument.proto;
72+
} else {
73+
proto.document = [self encodedDocument:existingDocument];
74+
}
7175
proto.hasCommittedMutations = existingDocument.hasCommittedMutations;
7276
} else if ([document isKindOfClass:[FSTUnknownDocument class]]) {
7377
FSTUnknownDocument *unknownDocument = (FSTUnknownDocument *)document;

‎Firestore/Source/Local/FSTLocalStore.mm

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -264,14 +264,24 @@ - (MaybeDocumentMap)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent {
264264
}
265265
}
266266

267-
// TODO(klimt): This could probably be an NSMutableDictionary.
268-
DocumentKeySet changedDocKeys;
267+
MaybeDocumentMap changedDocs;
269268
const DocumentKeySet &limboDocuments = remoteEvent.limboDocumentChanges;
269+
DocumentKeySet updatedKeys;
270+
for (const auto &kv : remoteEvent.documentUpdates) {
271+
updatedKeys = updatedKeys.insert(kv.first);
272+
}
273+
// Each loop iteration only affects its "own" doc, so it's safe to get all the remote
274+
// documents in advance in a single call.
275+
MaybeDocumentMap existingDocs = [self.remoteDocumentCache entriesForKeys:updatedKeys];
276+
270277
for (const auto &kv : remoteEvent.documentUpdates) {
271278
const DocumentKey &key = kv.first;
272279
FSTMaybeDocument *doc = kv.second;
273-
changedDocKeys = changedDocKeys.insert(key);
274-
FSTMaybeDocument *existingDoc = [self.remoteDocumentCache entryForKey:key];
280+
FSTMaybeDocument *existingDoc = nil;
281+
auto foundExisting = existingDocs.find(key);
282+
if (foundExisting != existingDocs.end()) {
283+
existingDoc = foundExisting->second;
284+
}
275285

276286
// If a document update isn't authoritative, make sure we don't apply an old document version
277287
// to the remote cache. We make an exception for SnapshotVersion.MIN which can happen for
@@ -280,6 +290,7 @@ - (MaybeDocumentMap)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent {
280290
(authoritativeUpdates.contains(doc.key) && !existingDoc.hasPendingWrites) ||
281291
doc.version >= existingDoc.version) {
282292
[self.remoteDocumentCache addEntry:doc];
293+
changedDocs = changedDocs.insert(key, doc);
283294
} else {
284295
LOG_DEBUG(
285296
"FSTLocalStore Ignoring outdated watch update for %s. "
@@ -306,7 +317,7 @@ - (MaybeDocumentMap)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent {
306317
[self.queryCache setLastRemoteSnapshotVersion:remoteVersion];
307318
}
308319

309-
return [self.localDocuments documentsForKeys:changedDocKeys];
320+
return [self.localDocuments localViewsForDocuments:changedDocs];
310321
});
311322
}
312323

‎Firestore/Source/Local/FSTMemoryRemoteDocumentCache.mm

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,16 @@ - (nullable FSTMaybeDocument *)entryForKey:(const DocumentKey &)key {
6868
return found != _docs.end() ? found->second : nil;
6969
}
7070

71+
- (MaybeDocumentMap)entriesForKeys:(const DocumentKeySet &)keys {
72+
MaybeDocumentMap results;
73+
for (const DocumentKey &key : keys) {
74+
// Make sure each key has a corresponding entry, which is null in case the document is not
75+
// found.
76+
results = results.insert(key, [self entryForKey:key]);
77+
}
78+
return results;
79+
}
80+
7181
- (DocumentMap)documentsMatchingQuery:(FSTQuery *)query {
7282
DocumentMap result;
7383

‎Firestore/Source/Local/FSTRemoteDocumentCache.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,16 @@ NS_ASSUME_NONNULL_BEGIN
5656
- (nullable FSTMaybeDocument *)entryForKey:
5757
(const firebase::firestore::model::DocumentKey &)documentKey;
5858

59+
/**
60+
* Looks up a set of entries in the cache.
61+
*
62+
* @param documentKeys The keys of the entries to look up.
63+
* @return The cached Document or NoDocument entries indexed by key. If an entry is not cached,
64+
* the corresponding key will be mapped to a null value.
65+
*/
66+
- (firebase::firestore::model::MaybeDocumentMap)entriesForKeys:
67+
(const firebase::firestore::model::DocumentKeySet &)documentKeys;
68+
5969
/**
6070
* Executes a query against the cached FSTDocument entries
6171
*

‎Firestore/Source/Model/FSTDocument.h

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include "Firestore/core/src/firebase/firestore/model/snapshot_version.h"
2222

2323
@class FSTFieldValue;
24+
@class GCFSDocument;
2425
@class FSTObjectValue;
2526

2627
NS_ASSUME_NONNULL_BEGIN
@@ -57,12 +58,24 @@ typedef NS_ENUM(NSInteger, FSTDocumentState) {
5758
version:(firebase::firestore::model::SnapshotVersion)version
5859
state:(FSTDocumentState)state;
5960

61+
+ (instancetype)documentWithData:(FSTObjectValue *)data
62+
key:(firebase::firestore::model::DocumentKey)key
63+
version:(firebase::firestore::model::SnapshotVersion)version
64+
state:(FSTDocumentState)state
65+
proto:(GCFSDocument *)proto;
66+
6067
- (nullable FSTFieldValue *)fieldForPath:(const firebase::firestore::model::FieldPath &)path;
6168
- (BOOL)hasLocalMutations;
6269
- (BOOL)hasCommittedMutations;
6370

6471
@property(nonatomic, strong, readonly) FSTObjectValue *data;
6572

73+
/**
74+
* Memoized serialized form of the document for optimization purposes (avoids repeated
75+
* serialization). Might be nil.
76+
*/
77+
@property(nonatomic, strong, readonly) GCFSDocument *proto;
78+
6679
@end
6780

6881
@interface FSTDeletedDocument : FSTMaybeDocument

‎Firestore/Source/Model/FSTDocument.mm

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,18 @@ + (instancetype)documentWithData:(FSTObjectValue *)data
8888
state:state];
8989
}
9090

91+
+ (instancetype)documentWithData:(FSTObjectValue *)data
92+
key:(DocumentKey)key
93+
version:(SnapshotVersion)version
94+
state:(FSTDocumentState)state
95+
proto:(GCFSDocument *)proto {
96+
return [[FSTDocument alloc] initWithData:data
97+
key:std::move(key)
98+
version:std::move(version)
99+
state:state
100+
proto:proto];
101+
}
102+
91103
- (instancetype)initWithData:(FSTObjectValue *)data
92104
key:(DocumentKey)key
93105
version:(SnapshotVersion)version
@@ -96,6 +108,21 @@ - (instancetype)initWithData:(FSTObjectValue *)data
96108
if (self) {
97109
_data = data;
98110
_documentState = state;
111+
_proto = nil;
112+
}
113+
return self;
114+
}
115+
116+
- (instancetype)initWithData:(FSTObjectValue *)data
117+
key:(DocumentKey)key
118+
version:(SnapshotVersion)version
119+
state:(FSTDocumentState)state
120+
proto:(GCFSDocument *)proto {
121+
self = [super initWithKey:std::move(key) version:std::move(version)];
122+
if (self) {
123+
_data = data;
124+
_documentState = state;
125+
_proto = proto;
99126
}
100127
return self;
101128
}

‎Firestore/Source/Remote/FSTSerializerBeta.mm

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,11 @@ - (FSTDocument *)decodedFoundDocument:(GCFSBatchGetDocumentsResponse *)response
438438
HARD_ASSERT(version != SnapshotVersion::None(),
439439
"Got a document response with no snapshot version");
440440

441-
return [FSTDocument documentWithData:value key:key version:version state:FSTDocumentStateSynced];
441+
return [FSTDocument documentWithData:value
442+
key:key
443+
version:version
444+
state:FSTDocumentStateSynced
445+
proto:response.found];
442446
}
443447

444448
- (FSTDeletedDocument *)decodedDeletedDocument:(GCFSBatchGetDocumentsResponse *)response {
@@ -1144,8 +1148,13 @@ - (FSTDocumentWatchChange *)decodedDocumentChange:(GCFSDocumentChange *)change {
11441148
const DocumentKey key = [self decodedDocumentKey:change.document.name];
11451149
SnapshotVersion version = [self decodedVersion:change.document.updateTime];
11461150
HARD_ASSERT(version != SnapshotVersion::None(), "Got a document change with no snapshot version");
1147-
FSTMaybeDocument *document =
1148-
[FSTDocument documentWithData:value key:key version:version state:FSTDocumentStateSynced];
1151+
// The document may soon be re-serialized back to protos in order to store it in local
1152+
// persistence. Memoize the encoded form to avoid encoding it again.
1153+
FSTMaybeDocument *document = [FSTDocument documentWithData:value
1154+
key:key
1155+
version:version
1156+
state:FSTDocumentStateSynced
1157+
proto:change.document];
11491158

11501159
NSArray<NSNumber *> *updatedTargetIds = [self decodedIntegerArray:change.targetIdsArray];
11511160
NSArray<NSNumber *> *removedTargetIds = [self decodedIntegerArray:change.removedTargetIdsArray];

‎Firestore/core/src/firebase/firestore/remote/watch_stream.mm

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,10 @@
8484
return status;
8585
}
8686

87-
LOG_DEBUG("%s response: %s", GetDebugDescription(),
88-
serializer_bridge_.Describe(response));
87+
if (bridge::IsLoggingEnabled()) {
88+
LOG_DEBUG("%s response: %s", GetDebugDescription(),
89+
serializer_bridge_.Describe(response));
90+
}
8991

9092
// A successful response means the stream is healthy.
9193
backoff_.Reset();

0 commit comments

Comments
 (0)