diff options
author | Konstantin Varlamov <var-const@users.noreply.github.com> | 2018-07-10 13:59:25 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-07-10 13:59:25 -0400 |
commit | ff95ffc61b6c6cf4b8ec69183d35e08497fbfd1a (patch) | |
tree | f70e21a1f04c5490cd5293091e0f24cd7f685bd2 | |
parent | 6d6ed82d4200125a0642ffc513cb6fe4e3c1b5d3 (diff) |
In FSTLocalDocumentsView, search for all batches affecting a set of keys in one go (#1505)
This uses the newly-added allMutationBatchesAffectingDocumentKeys to find/deserialize all such batches in one go and then reuse the batches while processing a set of keys.
-rw-r--r-- | Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm | 53 | ||||
-rw-r--r-- | Firestore/Source/Local/FSTLocalDocumentsView.mm | 43 | ||||
-rw-r--r-- | scripts/cpplint.py | 1 |
3 files changed, 85 insertions, 12 deletions
diff --git a/Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm b/Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm index 5340873..63616aa 100644 --- a/Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm @@ -17,6 +17,9 @@ #import <FirebaseFirestore/FirebaseFirestore.h> #import <XCTest/XCTest.h> +#include <mach/mach.h> + +#include <cstdint> #import "Firestore/Example/Tests/Util/FSTEventAccumulator.h" #import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h" @@ -323,4 +326,54 @@ [self awaitExpectations]; } +// Returns how much memory the test application is currently using, in megabytes (fractional part is +// truncated), or -1 if the OS call fails. +// TODO(varconst): move the helper function and the test into a new test target for performance +// testing. +int64_t GetCurrentMemoryUsedInMb() { + mach_task_basic_info taskInfo; + mach_msg_type_number_t taskInfoSize = MACH_TASK_BASIC_INFO_COUNT; + const auto errorCode = + task_info(mach_task_self(), MACH_TASK_BASIC_INFO, (task_info_t)&taskInfo, &taskInfoSize); + if (errorCode == KERN_SUCCESS) { + const int bytesInMegabyte = 1000 * 1000; + return taskInfo.resident_size / bytesInMegabyte; + } + return -1; +} + +- (void)testReasonableMemoryUsageForLotsOfMutations { + XCTestExpectation *expectation = + [self expectationWithDescription:@"testReasonableMemoryUsageForLotsOfMutations"]; + + FIRDocumentReference *mainDoc = [self documentRef]; + FIRWriteBatch *batch = [mainDoc.firestore batch]; + + // >= 500 mutations will be rejected, so use 500-1 mutations + for (int i = 0; i != 500 - 1; ++i) { + FIRDocumentReference *nestedDoc = [[mainDoc collectionWithPath:@"nested"] documentWithAutoID]; + // The exact data doesn't matter; what is important is the large number of mutations. + [batch setData:@{ + @"a" : @"foo", + @"b" : @"bar", + } + forDocument:nestedDoc]; + } + + const int64_t memoryUsedBeforeCommitMb = GetCurrentMemoryUsedInMb(); + XCTAssertNotEqual(memoryUsedBeforeCommitMb, -1); + [batch commitWithCompletion:^(NSError *_Nullable error) { + XCTAssertNil(error); + const int64_t memoryUsedAfterCommitMb = GetCurrentMemoryUsedInMb(); + XCTAssertNotEqual(memoryUsedAfterCommitMb, -1); + const int64_t memoryDeltaMb = memoryUsedAfterCommitMb - memoryUsedBeforeCommitMb; + // This by its nature cannot be a precise value. In debug mode, the increase on simulator + // seems to be around 90 MB. A regression would be on the scale of 500Mb. + XCTAssertLessThan(memoryDeltaMb, 150); + + [expectation fulfill]; + }]; + [self awaitExpectations]; +} + @end diff --git a/Firestore/Source/Local/FSTLocalDocumentsView.mm b/Firestore/Source/Local/FSTLocalDocumentsView.mm index 48c963e..4337879 100644 --- a/Firestore/Source/Local/FSTLocalDocumentsView.mm +++ b/Firestore/Source/Local/FSTLocalDocumentsView.mm @@ -62,21 +62,32 @@ NS_ASSUME_NONNULL_BEGIN } - (nullable FSTMaybeDocument *)documentForKey:(const DocumentKey &)key { + NSArray<FSTMutationBatch *> *batches = + [self.mutationQueue allMutationBatchesAffectingDocumentKey:key]; + return [self documentForKey:key inBatches:batches]; +} + +// Internal version of documentForKey: which allows reusing `batches`. +- (nullable FSTMaybeDocument *)documentForKey:(const DocumentKey &)key + inBatches:(NSArray<FSTMutationBatch *> *)batches { FSTMaybeDocument *_Nullable remoteDoc = [self.remoteDocumentCache entryForKey:key]; - return [self localDocument:remoteDoc key:key]; + return [self localDocument:remoteDoc key:key inBatches:batches]; } - (FSTMaybeDocumentDictionary *)documentsForKeys:(const DocumentKeySet &)keys { FSTMaybeDocumentDictionary *results = [FSTMaybeDocumentDictionary maybeDocumentDictionary]; + NSArray<FSTMutationBatch *> *batches = + [self.mutationQueue allMutationBatchesAffectingDocumentKeys:keys]; for (const DocumentKey &key : keys) { // TODO(mikelehen): PERF: Consider fetching all remote documents at once rather than one-by-one. - FSTMaybeDocument *maybeDoc = [self documentForKey:key]; + FSTMaybeDocument *maybeDoc = [self documentForKey:key inBatches:batches]; // TODO(http://b/32275378): Don't conflate missing / deleted. if (!maybeDoc) { maybeDoc = [FSTDeletedDocument documentWithKey:key version:SnapshotVersion::None()]; } results = [results dictionaryBySettingObject:maybeDoc forKey:key]; } + return results; } @@ -122,12 +133,13 @@ NS_ASSUME_NONNULL_BEGIN } // Now add in results for the matchingKeys. - for (const DocumentKey &key : matchingKeys) { - FSTMaybeDocument *doc = [self documentForKey:key]; - if ([doc isKindOfClass:[FSTDocument class]]) { - results = [results dictionaryBySettingObject:(FSTDocument *)doc forKey:key]; - } - } + FSTMaybeDocumentDictionary *matchingKeysDocs = [self documentsForKeys:matchingKeys]; + [matchingKeysDocs + enumerateKeysAndObjectsUsingBlock:^(FSTDocumentKey *key, FSTMaybeDocument *doc, BOOL *stop) { + if ([doc isKindOfClass:[FSTDocument class]]) { + results = [results dictionaryBySettingObject:(FSTDocument *)doc forKey:key]; + } + }]; // Finally, filter out any documents that don't actually match the query. Note that the extra // reference here prevents ARC from deallocating the initial unfiltered results while we're @@ -151,9 +163,8 @@ NS_ASSUME_NONNULL_BEGIN * @param documentKey The key of the document (necessary when remoteDocument is nil). */ - (nullable FSTMaybeDocument *)localDocument:(nullable FSTMaybeDocument *)document - key:(const DocumentKey &)documentKey { - NSArray<FSTMutationBatch *> *batches = - [self.mutationQueue allMutationBatchesAffectingDocumentKey:documentKey]; + key:(const DocumentKey &)documentKey + inBatches:(NSArray<FSTMutationBatch *> *)batches { for (FSTMutationBatch *batch in batches) { document = [batch applyTo:document documentKey:documentKey]; } @@ -169,10 +180,18 @@ NS_ASSUME_NONNULL_BEGIN * @return The local view of the documents. */ - (FSTDocumentDictionary *)localDocuments:(FSTDocumentDictionary *)documents { + __block DocumentKeySet keySet; + [documents + enumerateKeysAndObjectsUsingBlock:^(FSTDocumentKey *key, FSTDocument *doc, BOOL *stop) { + keySet = keySet.insert(doc.key); + }]; + NSArray<FSTMutationBatch *> *batches = + [self.mutationQueue allMutationBatchesAffectingDocumentKeys:keySet]; + __block FSTDocumentDictionary *result = documents; [documents enumerateKeysAndObjectsUsingBlock:^(FSTDocumentKey *key, FSTDocument *remoteDocument, BOOL *stop) { - FSTMaybeDocument *mutatedDoc = [self localDocument:remoteDocument key:key]; + FSTMaybeDocument *mutatedDoc = [self localDocument:remoteDocument key:key inBatches:batches]; if ([mutatedDoc isKindOfClass:[FSTDeletedDocument class]]) { result = [result dictionaryByRemovingObjectForKey:key]; } else if ([mutatedDoc isKindOfClass:[FSTDocument class]]) { diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 28f3761..f796ede 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -416,6 +416,7 @@ _CPP_HEADERS = frozenset([ _C_SYSTEM_DIRECTORIES = frozenset([ 'libkern', + 'mach', 'sys', ]) |