From ba07d1526a2ae74160754a3aad9377deb4e97907 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Wed, 3 Oct 2018 14:24:07 -0700 Subject: [PATCH 01/33] Wire together LRU GC --- .../Tests/Local/FSTLRUGarbageCollectorTests.h | 3 +- .../Local/FSTLRUGarbageCollectorTests.mm | 95 ++++++++++++++++++- .../FSTLevelDBLRUGarbageCollectorTests.mm | 4 +- .../FSTMemoryLRUGarbageCollectorTests.mm | 4 +- .../Tests/Local/FSTPersistenceTestHelpers.h | 11 +++ .../Tests/Local/FSTPersistenceTestHelpers.mm | 18 +++- Firestore/Source/Core/FSTFirestoreClient.mm | 46 ++++++++- .../Source/Local/FSTLRUGarbageCollector.h | 56 ++++++++++- .../Source/Local/FSTLRUGarbageCollector.mm | 94 ++++++++++++++++-- Firestore/Source/Local/FSTLevelDB.h | 9 +- Firestore/Source/Local/FSTLevelDB.mm | 24 +++-- Firestore/Source/Local/FSTLocalStore.h | 3 + Firestore/Source/Local/FSTLocalStore.mm | 4 + Firestore/Source/Local/FSTMemoryPersistence.h | 6 +- .../Source/Local/FSTMemoryPersistence.mm | 21 +++- Firestore/Source/Local/FSTPersistence.h | 2 +- .../Source/Public/FIRFirestoreSettings.h | 3 + Firestore/Source/Util/FSTDispatchQueue.h | 6 +- 18 files changed, 369 insertions(+), 40 deletions(-) diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h index 3861bdc53f8..5b5fa846e72 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h @@ -17,12 +17,13 @@ #import @protocol FSTPersistence; +struct FSTLruGcParams; NS_ASSUME_NONNULL_BEGIN @interface FSTLRUGarbageCollectorTests : XCTestCase -- (id)newPersistence; +- (id)newPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams; @end diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm index 86e5c315026..0356f313dbe 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm @@ -78,20 +78,28 @@ - (BOOL)isTestBaseClass { return ([self class] == [FSTLRUGarbageCollectorTests class]); } -- (void)newTestResources { +- (FSTLRUGarbageCollector *)garbageCollectorFromPersistence:(id)persistence { + return ((id)persistence.referenceDelegate).gc; +} + +- (void)newTestResourcesWithLruGcParams:(FSTLruGcParams)lruGcParams { HARD_ASSERT(_persistence == nil, "Persistence already created"); - _persistence = [self newPersistence]; + _persistence = [self newPersistenceWithLruGcParams:lruGcParams]; _queryCache = [_persistence queryCache]; _documentCache = [_persistence remoteDocumentCache]; _mutationQueue = [_persistence mutationQueueForUser:_user]; _initialSequenceNumber = _persistence.run("start querycache", [&]() -> ListenSequenceNumber { [_mutationQueue start]; - _gc = ((id)_persistence.referenceDelegate).gc; + _gc = [self garbageCollectorFromPersistence:_persistence]; return _persistence.currentSequenceNumber; }); } -- (id)newPersistence { +- (void)newTestResources { + [self newTestResourcesWithLruGcParams:FSTLruGcParams::Default()]; +} + +- (id)newPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams { @throw FSTAbstractMethodException(); // NOLINT } @@ -621,6 +629,7 @@ - (void)testRemoveTargetsThenGC { // Finally, do the garbage collection, up to but not including the removal of middleTarget NSDictionary *liveQueries = @{@(oldestTarget.targetID) : oldestTarget}; + int queriesRemoved = [self removeQueriesThroughSequenceNumber:upperBound liveQueries:liveQueries]; XCTAssertEqual(1, queriesRemoved, @"Expected to remove newest target"); int docsRemoved = [self removeOrphanedDocumentsThroughSequenceNumber:upperBound]; @@ -661,6 +670,84 @@ - (void)testGetsSize { [_persistence shutdown]; } +- (void)testDisabled { + if ([self isTestBaseClass]) return; + + FSTLruGcParams params = FSTLruGcParams::Disabled(); + [self newTestResourcesWithLruGcParams:params]; + + _persistence.run("fill cache", [&]() { + // Simulate a bunch of ack'd mutations + for (int i = 0; i < 500; i++) { + FSTDocument *doc = [self cacheADocumentInTransaction]; + [self markDocumentEligibleForGCInTransaction:doc.key]; + } + }); + + FSTLruGcResults results = [_gc tryRunGcWithLiveTargets:@{}]; + XCTAssertFalse(results.didRun); + + [_persistence shutdown]; +} + +- (void)testCacheTooSmall { + if ([self isTestBaseClass]) return; + + FSTLruGcParams params = FSTLruGcParams::Disabled(); + [self newTestResourcesWithLruGcParams:params]; + + _persistence.run("fill cache", [&]() { + // Simulate a bunch of ack'd mutations + for (int i = 0; i < 50; i++) { + FSTDocument *doc = [self cacheADocumentInTransaction]; + [self markDocumentEligibleForGCInTransaction:doc.key]; + } + }); + + size_t cacheSize = [_gc byteSize]; + // Verify that we don't have enough in our cache to warrant collection + XCTAssertLessThan(cacheSize, params.minBytesThreshold); + + // Try collection and verify that it didn't run + FSTLruGcResults results = [_gc tryRunGcWithLiveTargets:@{}]; + XCTAssertFalse(results.didRun); + + [_persistence shutdown]; +} + +- (void)testGCRan { + if ([self isTestBaseClass]) return; + + FSTLruGcParams params = FSTLruGcParams::Default(); + // Set a low threshold so we will definitely run + params.minBytesThreshold = 100; + [self newTestResourcesWithLruGcParams:params]; + + // Add 100 targets and 10 documents to each + for (int i = 0; i < 100; i++) { + // Use separate transactions so that each target and associated documents get their own + // sequence number. + _persistence.run("Add a target and some documents", [&]() { + FSTQueryData *queryData = [self addNextQueryInTransaction]; + for (int j = 0; j < 10; j++) { + FSTDocument *doc = [self cacheADocumentInTransaction]; + [self addDocument:doc.key toTarget:queryData.targetID]; + } + }); + } + + // Mark nothing as live, so everything is eligible. + FSTLruGcResults results = _persistence.run( + "GC", [&]() -> FSTLruGcResults { return [_gc tryRunGcWithLiveTargets:@{}]; }); + + // By default, we collect 10% of the sequence numbers. Since we added 100 targets, + // that should be 10 targets with 10 documents each, for a total of 100 documents. + XCTAssertTrue(results.didRun); + XCTAssertEqual(10, results.targetsRemoved); + XCTAssertEqual(100, results.documentsRemoved); + [_persistence shutdown]; +} + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Example/Tests/Local/FSTLevelDBLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLevelDBLRUGarbageCollectorTests.mm index e15d3f9d6f1..fa14a930526 100644 --- a/Firestore/Example/Tests/Local/FSTLevelDBLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLevelDBLRUGarbageCollectorTests.mm @@ -25,8 +25,8 @@ @interface FSTLevelDBLRUGarbageCollectorTests : FSTLRUGarbageCollectorTests @implementation FSTLevelDBLRUGarbageCollectorTests -- (id)newPersistence { - return [FSTPersistenceTestHelpers levelDBPersistence]; +- (id)newPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams { + return [FSTPersistenceTestHelpers levelDBPersistenceWithLruGcParams:lruGcParams]; } @end diff --git a/Firestore/Example/Tests/Local/FSTMemoryLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTMemoryLRUGarbageCollectorTests.mm index a21f306d610..14bd9004c3e 100644 --- a/Firestore/Example/Tests/Local/FSTMemoryLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTMemoryLRUGarbageCollectorTests.mm @@ -26,8 +26,8 @@ @interface FSTMemoryLRUGarbageCollectionTests : FSTLRUGarbageCollectorTests @implementation FSTMemoryLRUGarbageCollectionTests -- (id)newPersistence { - return [FSTPersistenceTestHelpers lruMemoryPersistence]; +- (id)newPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams { + return [FSTPersistenceTestHelpers lruMemoryPersistenceWithLruGcParams:lruGcParams]; } @end diff --git a/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h b/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h index 6b7a6060705..a28887d4b56 100644 --- a/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h +++ b/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h @@ -20,6 +20,7 @@ @class FSTLevelDB; @class FSTMemoryPersistence; +struct FSTLruGcParams; NS_ASSUME_NONNULL_BEGIN @@ -48,10 +49,20 @@ NS_ASSUME_NONNULL_BEGIN */ + (FSTLevelDB *)levelDBPersistenceWithDir:(firebase::firestore::util::Path)dir; +/** + * Creates and starts a new FSTLevelDB instance for testing, destroying any previous contents + * if they existed. + * + * Sets up the LRU garbage collection to use the provided params. + */ ++ (FSTLevelDB *)levelDBPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams; + /** Creates and starts a new FSTMemoryPersistence instance for testing. */ + (FSTMemoryPersistence *)eagerGCMemoryPersistence; + (FSTMemoryPersistence *)lruMemoryPersistence; + ++ (FSTMemoryPersistence *)lruMemoryPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams; @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm b/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm index c544ff6f05a..144914337db 100644 --- a/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm +++ b/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm @@ -61,8 +61,14 @@ + (Path)levelDBDir { } + (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir { + return [self levelDBPersistenceWithDir:dir lruGcParams:FSTLruGcParams::Default()]; +} + ++ (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir lruGcParams:(FSTLruGcParams)params { FSTLocalSerializer *serializer = [self localSerializer]; - FSTLevelDB *db = [[FSTLevelDB alloc] initWithDirectory:std::move(dir) serializer:serializer]; + FSTLevelDB *db = [[FSTLevelDB alloc] initWithDirectory:std::move(dir) + serializer:serializer + lruGcParams:params]; Status status = [db start]; if (!status.ok()) { [NSException raise:NSInternalInconsistencyException @@ -72,6 +78,10 @@ + (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir { return db; } ++ (FSTLevelDB *)levelDBPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams { + return [self levelDBPersistenceWithDir:[self levelDBDir] lruGcParams:lruGcParams]; +} + + (FSTLevelDB *)levelDBPersistence { return [self levelDBPersistenceWithDir:[self levelDBDir]]; } @@ -88,9 +98,13 @@ + (FSTMemoryPersistence *)eagerGCMemoryPersistence { } + (FSTMemoryPersistence *)lruMemoryPersistence { + return [self lruMemoryPersistenceWithLruGcParams:FSTLruGcParams::Default()]; +} + ++ (FSTMemoryPersistence *)lruMemoryPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams { FSTLocalSerializer *serializer = [self localSerializer]; FSTMemoryPersistence *persistence = - [FSTMemoryPersistence persistenceWithLRUGCAndSerializer:serializer]; + [FSTMemoryPersistence persistenceWithLruGcParams:lruGcParams serializer:serializer]; Status status = [persistence start]; if (!status.ok()) { [NSException raise:NSInternalInconsistencyException diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index af3a9b098cb..06463826239 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -31,6 +31,7 @@ #import "Firestore/Source/Core/FSTSyncEngine.h" #import "Firestore/Source/Core/FSTTransaction.h" #import "Firestore/Source/Core/FSTView.h" +#import "Firestore/Source/Local/FSTLRUGarbageCollector.h" #import "Firestore/Source/Local/FSTLevelDB.h" #import "Firestore/Source/Local/FSTLocalSerializer.h" #import "Firestore/Source/Local/FSTLocalStore.h" @@ -63,6 +64,11 @@ NS_ASSUME_NONNULL_BEGIN +/** How long we wait to try running LRU GC after SDK initialization. */ +static const long FSTLruGcInitialDelay = 60 * 1000; +/** Minimum amount of time between GC checks, after the first one. */ +static const long FSTLruGcRegularDelay = 5 * 60 * 1000; + @interface FSTFirestoreClient () { DatabaseInfo _databaseInfo; } @@ -96,6 +102,11 @@ - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo @implementation FSTFirestoreClient { std::unique_ptr _userExecutor; + long _initialGcDelay; + long _regularGcDelay; + BOOL _gcHasRun; + _Nullable id _lruDelegate; + FSTDelayedCallback *_Nullable _lruCallback; } - (Executor *)userExecutor { @@ -126,6 +137,9 @@ - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo _credentialsProvider = credentialsProvider; _userExecutor = std::move(userExecutor); _workerDispatchQueue = workerDispatchQueue; + _gcHasRun = NO; + _initialGcDelay = FSTLruGcInitialDelay; + _regularGcDelay = FSTLruGcRegularDelay; auto userPromise = std::make_shared>(); @@ -174,8 +188,13 @@ - (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistenc [[FSTSerializerBeta alloc] initWithDatabaseID:&self.databaseInfo->database_id()]; FSTLocalSerializer *serializer = [[FSTLocalSerializer alloc] initWithRemoteSerializer:remoteSerializer]; - - _persistence = [[FSTLevelDB alloc] initWithDirectory:std::move(dir) serializer:serializer]; + // TODO(gsoltis): enable LRU GC once API is finalized + FSTLevelDB *ldb = [[FSTLevelDB alloc] initWithDirectory:std::move(dir) + serializer:serializer + lruGcParams:FSTLruGcParams::Disabled()]; + _lruDelegate = ldb.referenceDelegate; + _persistence = ldb; + [self scheduleLruGarbageCollection]; } else { _persistence = [FSTMemoryPersistence persistenceWithEagerGC]; } @@ -217,6 +236,24 @@ - (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistenc [_remoteStore start]; } +/** + * Schedules a callback to try running LRU garbage collection. Reschedules itself after the GC has + * run. + */ +- (void)scheduleLruGarbageCollection { + long delay = _gcHasRun ? _initialGcDelay : _regularGcDelay; + _lruCallback = [_workerDispatchQueue + dispatchAfterDelay:delay + timerID:FSTLruGCTimer + block:^{ + FSTLruGcResults results = + [self->_localStore tryLruGarbageCollection:self->_lruDelegate.gc]; + self->_gcHasRun = YES; + LOG_DEBUG(results.ToString().c_str()); + [self scheduleLruGarbageCollection]; + }]; +} + - (void)credentialDidChangeWithUser:(const User &)user { [self.workerDispatchQueue verifyIsCurrentQueue]; @@ -251,6 +288,11 @@ - (void)shutdownWithCompletion:(nullable FSTVoidErrorBlock)completion { [self.workerDispatchQueue dispatchAsync:^{ self->_credentialsProvider->SetCredentialChangeListener(nullptr); + // If we've scheduled LRU garbage collection, cancel it. + FSTDelayedCallback *lruCallback = self->_lruCallback; + if (lruCallback != nil) { + [self->_lruCallback cancel]; + } [self.remoteStore shutdown]; [self.persistence shutdown]; if (completion) { diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index 39f41f3dd1e..c7c834fcb09 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -16,6 +16,7 @@ #import +#import "FIRFirestoreSettings.h" #import "Firestore/Source/Local/FSTQueryData.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/model/types.h" @@ -26,6 +27,22 @@ extern const firebase::firestore::model::ListenSequenceNumber kFSTListenSequenceNumberInvalid; +struct FSTLruGcParams { + static FSTLruGcParams Default() { + return FSTLruGcParams{.minBytesThreshold = 100 * 1024 * 1024, + .percentileToCollect = 10, + .maximumSequenceNumbersToCollect = 1000}; + } + + static FSTLruGcParams Disabled() { + return FSTLruGcParams{.minBytesThreshold = kFIRFirestorePersistenceCacheSizeUnlimited}; + } + + long minBytesThreshold; + int percentileToCollect; + int maximumSequenceNumbersToCollect; +}; + /** * Persistence layers intending to use LRU Garbage collection should implement this protocol. This * protocol defines the operations that the LRU garbage collector needs from the persistence layer. @@ -63,19 +80,52 @@ extern const firebase::firestore::model::ListenSequenceNumber kFSTListenSequence - (size_t)byteSize; +/** Returns the number of targets cached. */ +- (int32_t)targetCount; + +/** + * If available, persistence implementations should run any compaction that they can. This is done + * after GC has run to allow deletes to be processed and free up space. + */ +- (void)runPostCompaction; + /** Access to the underlying LRU Garbage collector instance. */ @property(strong, nonatomic, readonly) FSTLRUGarbageCollector *gc; @end +struct FSTLruGcResults { + static FSTLruGcResults DidNotRun() { + return FSTLruGcResults{.didRun = NO}; + } + + std::string ToString() const; + + long total_duration() const { + return targetCountDurationMs + upperBoundDurationMs + removedTargetsDurationMs + + removedDocumentsDurationMs + dbCompactionDurationMs; + } + + BOOL didRun; + + long targetCountDurationMs; + long upperBoundDurationMs; + long removedTargetsDurationMs; + long removedDocumentsDurationMs; + long dbCompactionDurationMs; + + int sequenceNumbersCollected; + int targetsRemoved; + int documentsRemoved; +}; + /** * FSTLRUGarbageCollector defines the LRU algorithm used to clean up old documents and targets. It * is persistence-agnostic, as long as proper delegate is provided. */ @interface FSTLRUGarbageCollector : NSObject -- (instancetype)initWithQueryCache:(id)queryCache - delegate:(id)delegate; +- (instancetype)initWithDelegate:(id)delegate params:(FSTLruGcParams)params; /** * Given a target percentile, return the number of queries that make up that percentage of the @@ -107,4 +157,6 @@ extern const firebase::firestore::model::ListenSequenceNumber kFSTListenSequence - (size_t)byteSize; +- (FSTLruGcResults)tryRunGcWithLiveTargets:(NSDictionary *)liveTargets; + @end diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 7f9b6b93d0c..2b763a22402 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -19,14 +19,27 @@ #include #import "Firestore/Source/Local/FSTMutationQueue.h" +#import "Firestore/Source/Local/FSTPersistence.h" #import "Firestore/Source/Local/FSTQueryCache.h" +#include "Firestore/core/include/firebase/firestore/timestamp.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" +#include "Firestore/core/src/firebase/firestore/util/log.h" +using firebase::Timestamp; using firebase::firestore::model::DocumentKey; using firebase::firestore::model::ListenSequenceNumber; +const long kFIRFirestorePersistenceCacheSizeUnlimited = -1; const ListenSequenceNumber kFSTListenSequenceNumberInvalid = -1; +static long toMilliseconds(const Timestamp &ts) { + return (1000 * ts.seconds()) + (ts.nanoseconds() / 1000000); +} + +static long millisecondsBetween(const Timestamp &start, const Timestamp &end) { + return toMilliseconds(end) - toMilliseconds(start); +} + /** * RollingSequenceNumberBuffer tracks the nth sequence number in a series. Sequence numbers may be * added out of order. @@ -66,28 +79,91 @@ size_t size() const { const size_t max_elements_; }; -@interface FSTLRUGarbageCollector () - -@property(nonatomic, strong, readonly) id queryCache; - -@end +std::string FSTLruGcResults::ToString() const { + if (!didRun) { + return "Garbage Collection skipped"; + } else { + std::string desc = "LRU Garbage Collection:\n"; + absl::StrAppend(&desc, "\tCounted targets in ", targetCountDurationMs, "ms\n"); + absl::StrAppend(&desc, "\tDetermined least recently used ", sequenceNumbersCollected, + " sequence numbers in ", upperBoundDurationMs, "ms\n"); + absl::StrAppend(&desc, "\tRemoved ", targetsRemoved, " targets in ", removedTargetsDurationMs, + "ms\n"); + absl::StrAppend(&desc, "\tRemoved ", documentsRemoved, " documents in ", + removedDocumentsDurationMs, "ms\n"); + absl::StrAppend(&desc, "\tCompacted leveldb database in ", dbCompactionDurationMs, "ms\n"); + absl::StrAppend(&desc, "Total duration: ", total_duration(), "ms"); + return desc; + } +} @implementation FSTLRUGarbageCollector { id _delegate; + FSTLruGcParams _params; + long _startTime; + long _lastRunTime; } -- (instancetype)initWithQueryCache:(id)queryCache - delegate:(id)delegate { +- (instancetype)initWithDelegate:(id)delegate params:(FSTLruGcParams)params { self = [super init]; if (self) { - _queryCache = queryCache; _delegate = delegate; + _params = std::move(params); + _lastRunTime = -1; } return self; } +- (FSTLruGcResults)tryRunGcWithLiveTargets:(NSDictionary *)liveTargets { + if (_params.minBytesThreshold == kFIRFirestorePersistenceCacheSizeUnlimited) { + return FSTLruGcResults::DidNotRun(); + } + + size_t currentSize = [self byteSize]; + if (currentSize < _params.minBytesThreshold) { + // Not enough on disk to warrant collection. Wait another timeout cycle. + return FSTLruGcResults::DidNotRun(); + } else { + return [self runGCWithLiveTargets:liveTargets]; + } +} + +- (FSTLruGcResults)runGCWithLiveTargets:(NSDictionary *)liveTargets { + Timestamp start = Timestamp::Now(); + int sequenceNumbers = [self queryCountForPercentile:_params.percentileToCollect]; + // Cap at the configured max + if (sequenceNumbers > _params.maximumSequenceNumbersToCollect) { + sequenceNumbers = _params.maximumSequenceNumbersToCollect; + } + Timestamp countedTargets = Timestamp::Now(); + + ListenSequenceNumber upperBound = [self sequenceNumberForQueryCount:sequenceNumbers]; + Timestamp foundUpperBound = Timestamp::Now(); + + int numTargetsRemoved = + [self removeQueriesUpThroughSequenceNumber:upperBound liveQueries:liveTargets]; + Timestamp removedTargets = Timestamp::Now(); + + int numDocumentsRemoved = [self removeOrphanedDocumentsThroughSequenceNumber:upperBound]; + Timestamp removedDocuments = Timestamp::Now(); + + [_delegate runPostCompaction]; + Timestamp compactedDb = Timestamp::Now(); + + return FSTLruGcResults{ + .didRun = YES, + .targetCountDurationMs = millisecondsBetween(start, countedTargets), + .upperBoundDurationMs = millisecondsBetween(countedTargets, foundUpperBound), + .removedTargetsDurationMs = millisecondsBetween(foundUpperBound, removedTargets), + .removedDocumentsDurationMs = millisecondsBetween(removedTargets, removedDocuments), + .dbCompactionDurationMs = millisecondsBetween(removedDocuments, compactedDb), + .sequenceNumbersCollected = sequenceNumbers, + .targetsRemoved = numTargetsRemoved, + .documentsRemoved = numDocumentsRemoved}; +} + - (int)queryCountForPercentile:(NSUInteger)percentile { - int totalCount = [self.queryCache count]; + int totalCount = [_delegate targetCount]; int setSize = (int)((percentile / 100.0f) * totalCount); return setSize; } diff --git a/Firestore/Source/Local/FSTLevelDB.h b/Firestore/Source/Local/FSTLevelDB.h index 947397e19b7..1cc04c672e3 100644 --- a/Firestore/Source/Local/FSTLevelDB.h +++ b/Firestore/Source/Local/FSTLevelDB.h @@ -20,6 +20,7 @@ #include #include +#import "Firestore/Source/Local/FSTLRUGarbageCollector.h" #import "Firestore/Source/Local/FSTPersistence.h" #include "Firestore/core/src/firebase/firestore/core/database_info.h" #include "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h" @@ -31,6 +32,9 @@ NS_ASSUME_NONNULL_BEGIN +@interface FSTLevelDBLRUDelegate : NSObject +@end + /** A LevelDB-backed instance of FSTPersistence. */ // TODO(mikelehen): Rename to FSTLevelDBPersistence. @interface FSTLevelDB : NSObject @@ -40,7 +44,8 @@ NS_ASSUME_NONNULL_BEGIN * opening any database files is deferred until -[FSTPersistence start] is called. */ - (instancetype)initWithDirectory:(firebase::firestore::util::Path)directory - serializer:(FSTLocalSerializer *)serializer NS_DESIGNATED_INITIALIZER; + serializer:(FSTLocalSerializer *)serializer + lruGcParams:(FSTLruGcParams)lruGcParams NS_DESIGNATED_INITIALIZER; - (instancetype)init NS_UNAVAILABLE; @@ -80,6 +85,8 @@ NS_ASSUME_NONNULL_BEGIN @property(nonatomic, readonly) const std::set &users; +@property(nonatomic, readonly, strong) FSTLevelDBLRUDelegate *referenceDelegate; + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Local/FSTLevelDB.mm b/Firestore/Source/Local/FSTLevelDB.mm index 8501c44b53b..fc5791f95cc 100644 --- a/Firestore/Source/Local/FSTLevelDB.mm +++ b/Firestore/Source/Local/FSTLevelDB.mm @@ -92,7 +92,7 @@ - (size_t)byteSize; * Although this could implement FSTTransactional, it doesn't because it is not directly tied to * a transaction runner, it just happens to be called from FSTLevelDB, which is FSTTransactional. */ -@interface FSTLevelDBLRUDelegate : NSObject +@interface FSTLevelDBLRUDelegate () - (void)transactionWillStart; @@ -112,10 +112,10 @@ @implementation FSTLevelDBLRUDelegate { FSTListenSequence *_listenSequence; } -- (instancetype)initWithPersistence:(FSTLevelDB *)persistence { +- (instancetype)initWithPersistence:(FSTLevelDB *)persistence + lruGcParams:(FSTLruGcParams)lruGcParams { if (self = [super init]) { - _gc = - [[FSTLRUGarbageCollector alloc] initWithQueryCache:[persistence queryCache] delegate:self]; + _gc = [[FSTLRUGarbageCollector alloc] initWithDelegate:self params:lruGcParams]; _db = persistence; _currentSequenceNumber = kFSTListenSequenceNumberInvalid; } @@ -224,6 +224,15 @@ - (int)removeTargetsThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber return [queryCache removeQueriesThroughSequenceNumber:sequenceNumber liveQueries:liveQueries]; } +- (int32_t)targetCount { + return [_db.queryCache count]; +} + +- (void)runPostCompaction { + // Compacts the entire db + _db.ptr->CompactRange(NULL, NULL); +} + - (FSTLRUGarbageCollector *)gc { return _gc; } @@ -285,12 +294,15 @@ + (const ReadOptions)standardReadOptions { return users; } -- (instancetype)initWithDirectory:(Path)directory serializer:(FSTLocalSerializer *)serializer { +- (instancetype)initWithDirectory:(Path)directory + serializer:(FSTLocalSerializer *)serializer + lruGcParams:(FSTLruGcParams)lruGcParams { if (self = [super init]) { _directory = std::move(directory); _serializer = serializer; _queryCache = [[FSTLevelDBQueryCache alloc] initWithDB:self serializer:self.serializer]; - _referenceDelegate = [[FSTLevelDBLRUDelegate alloc] initWithPersistence:self]; + _referenceDelegate = + [[FSTLevelDBLRUDelegate alloc] initWithPersistence:self lruGcParams:lruGcParams]; _transactionRunner.SetBackingPersistence(self); } return self; diff --git a/Firestore/Source/Local/FSTLocalStore.h b/Firestore/Source/Local/FSTLocalStore.h index 6978afa1250..580a137b983 100644 --- a/Firestore/Source/Local/FSTLocalStore.h +++ b/Firestore/Source/Local/FSTLocalStore.h @@ -26,6 +26,7 @@ @class FSTLocalViewChanges; @class FSTLocalWriteResult; +@class FSTLRUGarbageCollector; @class FSTMutation; @class FSTMutationBatch; @class FSTMutationBatchResult; @@ -176,6 +177,8 @@ NS_ASSUME_NONNULL_BEGIN - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID: (firebase::firestore::model::BatchId)batchID; +- (FSTLruGcResults)tryLruGarbageCollection:(FSTLRUGarbageCollector *)garbageCollector; + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index 709ec545a96..d51763ee7fe 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -563,6 +563,10 @@ - (void)applyBatchResult:(FSTMutationBatchResult *)batchResult { } } +- (FSTLruGcResults)tryLruGarbageCollection:(FSTLRUGarbageCollector *)garbageCollector { + return [garbageCollector tryRunGcWithLiveTargets:_targetIDs]; +} + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Local/FSTMemoryPersistence.h b/Firestore/Source/Local/FSTMemoryPersistence.h index 94ee875f155..1c535d185ef 100644 --- a/Firestore/Source/Local/FSTMemoryPersistence.h +++ b/Firestore/Source/Local/FSTMemoryPersistence.h @@ -32,7 +32,8 @@ NS_ASSUME_NONNULL_BEGIN + (instancetype)persistenceWithEagerGC; -+ (instancetype)persistenceWithLRUGCAndSerializer:(FSTLocalSerializer *)serializer; ++ (instancetype)persistenceWithLruGcParams:(FSTLruGcParams)lruGcParams + serializer:(FSTLocalSerializer *)serializer; @end @@ -52,7 +53,8 @@ NS_ASSUME_NONNULL_BEGIN : NSObject - (instancetype)initWithPersistence:(FSTMemoryPersistence *)persistence - serializer:(FSTLocalSerializer *)serializer; + serializer:(FSTLocalSerializer *)serializer + lruGcParams:(FSTLruGcParams)lruGcParams; - (BOOL)isPinnedAtSequenceNumber:(firebase::firestore::model::ListenSequenceNumber)upperBound document:(const firebase::firestore::model::DocumentKey &)key; diff --git a/Firestore/Source/Local/FSTMemoryPersistence.mm b/Firestore/Source/Local/FSTMemoryPersistence.mm index 0d0dcd50d77..656ff7bdac8 100644 --- a/Firestore/Source/Local/FSTMemoryPersistence.mm +++ b/Firestore/Source/Local/FSTMemoryPersistence.mm @@ -83,10 +83,13 @@ + (instancetype)persistenceWithEagerGC { return persistence; } -+ (instancetype)persistenceWithLRUGCAndSerializer:(FSTLocalSerializer *)serializer { ++ (instancetype)persistenceWithLruGcParams:(FSTLruGcParams)lruGcParams + serializer:(FSTLocalSerializer *)serializer { FSTMemoryPersistence *persistence = [[FSTMemoryPersistence alloc] init]; persistence.referenceDelegate = - [[FSTMemoryLRUReferenceDelegate alloc] initWithPersistence:persistence serializer:serializer]; + [[FSTMemoryLRUReferenceDelegate alloc] initWithPersistence:persistence + serializer:serializer + lruGcParams:lruGcParams]; return persistence; } @@ -163,11 +166,11 @@ @implementation FSTMemoryLRUReferenceDelegate { } - (instancetype)initWithPersistence:(FSTMemoryPersistence *)persistence - serializer:(FSTLocalSerializer *)serializer { + serializer:(FSTLocalSerializer *)serializer + lruGcParams:(FSTLruGcParams)lruGcParams { if (self = [super init]) { _persistence = persistence; - _gc = - [[FSTLRUGarbageCollector alloc] initWithQueryCache:[_persistence queryCache] delegate:self]; + _gc = [[FSTLRUGarbageCollector alloc] initWithDelegate:self params:lruGcParams]; _currentSequenceNumber = kFSTListenSequenceNumberInvalid; // Theoretically this is always 0, since this is all in-memory... ListenSequenceNumber highestSequenceNumber = @@ -235,6 +238,14 @@ - (int)removeTargetsThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber liveQueries:liveQueries]; } +- (int32_t)targetCount { + return [_persistence.queryCache count]; +} + +- (void)runPostCompaction { + // No-op for memory persistence. +} + - (int)removeOrphanedDocumentsThroughSequenceNumber:(ListenSequenceNumber)upperBound { return [(FSTMemoryRemoteDocumentCache *)_persistence.remoteDocumentCache removeOrphanedDocuments:self diff --git a/Firestore/Source/Local/FSTPersistence.h b/Firestore/Source/Local/FSTPersistence.h index 088d43d465a..6d481b972ad 100644 --- a/Firestore/Source/Local/FSTPersistence.h +++ b/Firestore/Source/Local/FSTPersistence.h @@ -124,7 +124,7 @@ NS_ASSUME_NONNULL_BEGIN * Implementations that care about sequence numbers are responsible for generating them and making * them available. */ -@protocol FSTReferenceDelegate +@protocol FSTReferenceDelegate /** * Registers an FSTReferenceSet of documents that should be considered 'referenced' and not eligible diff --git a/Firestore/Source/Public/FIRFirestoreSettings.h b/Firestore/Source/Public/FIRFirestoreSettings.h index cd3f91ced57..49a4282ab30 100644 --- a/Firestore/Source/Public/FIRFirestoreSettings.h +++ b/Firestore/Source/Public/FIRFirestoreSettings.h @@ -18,6 +18,9 @@ NS_ASSUME_NONNULL_BEGIN +/** Used to set on-disk cache size to unlimited. Garbage collection will not run. */ +extern const long kFIRFirestorePersistenceCacheSizeUnlimited; + /** Settings used to configure a `FIRFirestore` instance. */ NS_SWIFT_NAME(FirestoreSettings) @interface FIRFirestoreSettings : NSObject diff --git a/Firestore/Source/Util/FSTDispatchQueue.h b/Firestore/Source/Util/FSTDispatchQueue.h index 7f9d6f2a9a6..8c550a29f4f 100644 --- a/Firestore/Source/Util/FSTDispatchQueue.h +++ b/Firestore/Source/Util/FSTDispatchQueue.h @@ -42,7 +42,11 @@ typedef NS_ENUM(NSInteger, FSTTimerID) { * A timer used in FSTOnlineStateTracker to transition from OnlineState Unknown to Offline * after a set timeout, rather than waiting indefinitely for success or failure. */ - FSTTimerIDOnlineStateTimeout + FSTTimerIDOnlineStateTimeout, + /** + * A timer used to periodically attempt LRU Garbage collection + */ + FSTLruGCTimer }; /** From 46b6a95a372df6d2332f181b99af005495ea0897 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Wed, 3 Oct 2018 14:45:01 -0700 Subject: [PATCH 02/33] Lint --- Firestore/Source/Local/FSTLRUGarbageCollector.h | 2 ++ Firestore/Source/Local/FSTLRUGarbageCollector.mm | 1 + 2 files changed, 3 insertions(+) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index c7c834fcb09..473159b5824 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -16,6 +16,8 @@ #import +#include + #import "FIRFirestoreSettings.h" #import "Firestore/Source/Local/FSTQueryData.h" #include "Firestore/core/src/firebase/firestore/model/document_key.h" diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 2b763a22402..962c65b4196 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -17,6 +17,7 @@ #import "Firestore/Source/Local/FSTLRUGarbageCollector.h" #include +#include #import "Firestore/Source/Local/FSTMutationQueue.h" #import "Firestore/Source/Local/FSTPersistence.h" From ab90dd6f63a8a5d1c207091ed14dceaddb5ae4e5 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Wed, 3 Oct 2018 15:33:01 -0700 Subject: [PATCH 03/33] Fix declarations --- Firestore/Source/Local/FSTLocalStore.h | 1 + Firestore/Source/Local/FSTLocalStore.mm | 1 + 2 files changed, 2 insertions(+) diff --git a/Firestore/Source/Local/FSTLocalStore.h b/Firestore/Source/Local/FSTLocalStore.h index 580a137b983..548d1aaedcf 100644 --- a/Firestore/Source/Local/FSTLocalStore.h +++ b/Firestore/Source/Local/FSTLocalStore.h @@ -27,6 +27,7 @@ @class FSTLocalViewChanges; @class FSTLocalWriteResult; @class FSTLRUGarbageCollector; +struct FSTLruGcResults; @class FSTMutation; @class FSTMutationBatch; @class FSTMutationBatchResult; diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index d51763ee7fe..ffbdeb3a48f 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -21,6 +21,7 @@ #import "FIRTimestamp.h" #import "Firestore/Source/Core/FSTListenSequence.h" #import "Firestore/Source/Core/FSTQuery.h" +#import "Firestore/Source/Local/FSTLRUGarbageCollector.h" #import "Firestore/Source/Local/FSTLocalDocumentsView.h" #import "Firestore/Source/Local/FSTLocalViewChanges.h" #import "Firestore/Source/Local/FSTLocalWriteResult.h" From b4ec0dc6b16d19a2718021841cd4f94025068396 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Thu, 4 Oct 2018 16:09:35 -0700 Subject: [PATCH 04/33] Renaming, and using C++ namespaces --- .../Tests/Local/FSTLRUGarbageCollectorTests.h | 5 ++-- .../Local/FSTLRUGarbageCollectorTests.mm | 21 +++++++++-------- .../FSTLevelDBLRUGarbageCollectorTests.mm | 7 ++++-- .../FSTMemoryLRUGarbageCollectorTests.mm | 7 ++++-- .../Tests/Local/FSTPersistenceTestHelpers.h | 7 +++--- .../Tests/Local/FSTPersistenceTestHelpers.mm | 21 +++++++++-------- Firestore/Source/Core/FSTFirestoreClient.mm | 5 ++-- .../Source/Local/FSTLRUGarbageCollector.h | 23 ++++++++++++------- .../Source/Local/FSTLRUGarbageCollector.mm | 5 ++-- Firestore/Source/Local/FSTLevelDB.h | 3 ++- Firestore/Source/Local/FSTLevelDB.mm | 12 +++++----- Firestore/Source/Local/FSTMemoryPersistence.h | 6 ++--- .../Source/Local/FSTMemoryPersistence.mm | 11 +++++---- Firestore/Source/Util/FSTDispatchQueue.h | 2 +- 14 files changed, 78 insertions(+), 57 deletions(-) diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h index 5b5fa846e72..13fcd723c7e 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h @@ -16,14 +16,15 @@ #import +#import "Firestore/Source/Local/FSTLRUGarbageCollector.h" + @protocol FSTPersistence; -struct FSTLruGcParams; NS_ASSUME_NONNULL_BEGIN @interface FSTLRUGarbageCollectorTests : XCTestCase -- (id)newPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams; +- (id)newPersistenceWithLruParams:(firebase::firestore::local::LruParams)lruParams; @end diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm index 0356f313dbe..b8381ff9138 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm @@ -40,6 +40,7 @@ namespace testutil = firebase::firestore::testutil; using firebase::firestore::auth::User; +using firebase::firestore::local::LruParams; using firebase::firestore::model::DocumentKey; using firebase::firestore::model::DocumentKeyHash; using firebase::firestore::model::DocumentKeySet; @@ -82,9 +83,9 @@ - (FSTLRUGarbageCollector *)garbageCollectorFromPersistence:(id) return ((id)persistence.referenceDelegate).gc; } -- (void)newTestResourcesWithLruGcParams:(FSTLruGcParams)lruGcParams { +- (void)newTestResourcesWithLruParams:(LruParams)lruParams { HARD_ASSERT(_persistence == nil, "Persistence already created"); - _persistence = [self newPersistenceWithLruGcParams:lruGcParams]; + _persistence = [self newPersistenceWithLruParams:lruParams]; _queryCache = [_persistence queryCache]; _documentCache = [_persistence remoteDocumentCache]; _mutationQueue = [_persistence mutationQueueForUser:_user]; @@ -96,10 +97,10 @@ - (void)newTestResourcesWithLruGcParams:(FSTLruGcParams)lruGcParams { } - (void)newTestResources { - [self newTestResourcesWithLruGcParams:FSTLruGcParams::Default()]; + [self newTestResourcesWithLruParams:LruParams::Default()]; } -- (id)newPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams { +- (id)newPersistenceWithLruParams:(LruParams)lruParams { @throw FSTAbstractMethodException(); // NOLINT } @@ -673,8 +674,8 @@ - (void)testGetsSize { - (void)testDisabled { if ([self isTestBaseClass]) return; - FSTLruGcParams params = FSTLruGcParams::Disabled(); - [self newTestResourcesWithLruGcParams:params]; + LruParams params = LruParams::Disabled(); + [self newTestResourcesWithLruParams:params]; _persistence.run("fill cache", [&]() { // Simulate a bunch of ack'd mutations @@ -693,8 +694,8 @@ - (void)testDisabled { - (void)testCacheTooSmall { if ([self isTestBaseClass]) return; - FSTLruGcParams params = FSTLruGcParams::Disabled(); - [self newTestResourcesWithLruGcParams:params]; + LruParams params = LruParams::Disabled(); + [self newTestResourcesWithLruParams:params]; _persistence.run("fill cache", [&]() { // Simulate a bunch of ack'd mutations @@ -718,10 +719,10 @@ - (void)testCacheTooSmall { - (void)testGCRan { if ([self isTestBaseClass]) return; - FSTLruGcParams params = FSTLruGcParams::Default(); + LruParams params = LruParams::Default(); // Set a low threshold so we will definitely run params.minBytesThreshold = 100; - [self newTestResourcesWithLruGcParams:params]; + [self newTestResourcesWithLruParams:params]; // Add 100 targets and 10 documents to each for (int i = 0; i < 100; i++) { diff --git a/Firestore/Example/Tests/Local/FSTLevelDBLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLevelDBLRUGarbageCollectorTests.mm index fa14a930526..494dccfa2c9 100644 --- a/Firestore/Example/Tests/Local/FSTLevelDBLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLevelDBLRUGarbageCollectorTests.mm @@ -16,8 +16,11 @@ #import "Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h" #import "Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h" +#import "Firestore/Source/Local/FSTLRUGarbageCollector.h" #import "Firestore/Source/Local/FSTLevelDB.h" +using firebase::firestore::local::LruParams; + NS_ASSUME_NONNULL_BEGIN @interface FSTLevelDBLRUGarbageCollectorTests : FSTLRUGarbageCollectorTests @@ -25,8 +28,8 @@ @interface FSTLevelDBLRUGarbageCollectorTests : FSTLRUGarbageCollectorTests @implementation FSTLevelDBLRUGarbageCollectorTests -- (id)newPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams { - return [FSTPersistenceTestHelpers levelDBPersistenceWithLruGcParams:lruGcParams]; +- (id)newPersistenceWithLruParams:(LruParams)lruParams { + return [FSTPersistenceTestHelpers levelDBPersistenceWithLruParams:lruParams]; } @end diff --git a/Firestore/Example/Tests/Local/FSTMemoryLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTMemoryLRUGarbageCollectorTests.mm index 14bd9004c3e..5b86ebb02ea 100644 --- a/Firestore/Example/Tests/Local/FSTMemoryLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTMemoryLRUGarbageCollectorTests.mm @@ -17,8 +17,11 @@ #import "Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h" #import "Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h" +#import "Firestore/Source/Local/FSTLRUGarbageCollector.h" #import "Firestore/Source/Local/FSTMemoryPersistence.h" +using firebase::firestore::local::LruParams; + NS_ASSUME_NONNULL_BEGIN @interface FSTMemoryLRUGarbageCollectionTests : FSTLRUGarbageCollectorTests @@ -26,8 +29,8 @@ @interface FSTMemoryLRUGarbageCollectionTests : FSTLRUGarbageCollectorTests @implementation FSTMemoryLRUGarbageCollectionTests -- (id)newPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams { - return [FSTPersistenceTestHelpers lruMemoryPersistenceWithLruGcParams:lruGcParams]; +- (id)newPersistenceWithLruParams:(LruParams)lruParams { + return [FSTPersistenceTestHelpers lruMemoryPersistenceWithLruParams:lruParams]; } @end diff --git a/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h b/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h index a28887d4b56..8d5937afc67 100644 --- a/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h +++ b/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h @@ -16,11 +16,11 @@ #import +#import "Firestore/Source/Local/FSTLRUGarbageCollector.h" #include "Firestore/core/src/firebase/firestore/util/path.h" @class FSTLevelDB; @class FSTMemoryPersistence; -struct FSTLruGcParams; NS_ASSUME_NONNULL_BEGIN @@ -55,14 +55,15 @@ NS_ASSUME_NONNULL_BEGIN * * Sets up the LRU garbage collection to use the provided params. */ -+ (FSTLevelDB *)levelDBPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams; ++ (FSTLevelDB *)levelDBPersistenceWithLruParams:(firebase::firestore::local::LruParams)lruParams; /** Creates and starts a new FSTMemoryPersistence instance for testing. */ + (FSTMemoryPersistence *)eagerGCMemoryPersistence; + (FSTMemoryPersistence *)lruMemoryPersistence; -+ (FSTMemoryPersistence *)lruMemoryPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams; ++ (FSTMemoryPersistence *)lruMemoryPersistenceWithLruParams: + (firebase::firestore::local::LruParams)lruParams; @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm b/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm index 144914337db..74059c33d61 100644 --- a/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm +++ b/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm @@ -18,6 +18,7 @@ #include +#import "Firestore/Source/Local/FSTLRUGarbageCollector.h" #import "Firestore/Source/Local/FSTLevelDB.h" #import "Firestore/Source/Local/FSTLocalSerializer.h" #import "Firestore/Source/Local/FSTMemoryPersistence.h" @@ -30,6 +31,7 @@ #include "Firestore/core/src/firebase/firestore/util/string_apple.h" namespace util = firebase::firestore::util; +using firebase::firestore::local::LruParams; using firebase::firestore::model::DatabaseId; using firebase::firestore::util::Path; using firebase::firestore::util::Status; @@ -61,14 +63,13 @@ + (Path)levelDBDir { } + (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir { - return [self levelDBPersistenceWithDir:dir lruGcParams:FSTLruGcParams::Default()]; + return [self levelDBPersistenceWithDir:dir lruParams:LruParams::Default()]; } -+ (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir lruGcParams:(FSTLruGcParams)params { ++ (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir lruParams:(LruParams)params { FSTLocalSerializer *serializer = [self localSerializer]; - FSTLevelDB *db = [[FSTLevelDB alloc] initWithDirectory:std::move(dir) - serializer:serializer - lruGcParams:params]; + FSTLevelDB *db = + [[FSTLevelDB alloc] initWithDirectory:std::move(dir) serializer:serializer lruParams:params]; Status status = [db start]; if (!status.ok()) { [NSException raise:NSInternalInconsistencyException @@ -78,8 +79,8 @@ + (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir lruGcParams:(FSTLruGcParams) return db; } -+ (FSTLevelDB *)levelDBPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams { - return [self levelDBPersistenceWithDir:[self levelDBDir] lruGcParams:lruGcParams]; ++ (FSTLevelDB *)levelDBPersistenceWithLruParams:(LruParams)lruParams { + return [self levelDBPersistenceWithDir:[self levelDBDir] lruParams:lruParams]; } + (FSTLevelDB *)levelDBPersistence { @@ -98,13 +99,13 @@ + (FSTMemoryPersistence *)eagerGCMemoryPersistence { } + (FSTMemoryPersistence *)lruMemoryPersistence { - return [self lruMemoryPersistenceWithLruGcParams:FSTLruGcParams::Default()]; + return [self lruMemoryPersistenceWithLruParams:LruParams::Default()]; } -+ (FSTMemoryPersistence *)lruMemoryPersistenceWithLruGcParams:(FSTLruGcParams)lruGcParams { ++ (FSTMemoryPersistence *)lruMemoryPersistenceWithLruParams:(LruParams)lruParams { FSTLocalSerializer *serializer = [self localSerializer]; FSTMemoryPersistence *persistence = - [FSTMemoryPersistence persistenceWithLruGcParams:lruGcParams serializer:serializer]; + [FSTMemoryPersistence persistenceWithLruParams:lruParams serializer:serializer]; Status status = [persistence start]; if (!status.ok()) { [NSException raise:NSInternalInconsistencyException diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 06463826239..aab245ed436 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -55,6 +55,7 @@ using firebase::firestore::auth::CredentialsProvider; using firebase::firestore::auth::User; using firebase::firestore::core::DatabaseInfo; +using firebase::firestore::local::LruParams; using firebase::firestore::model::DatabaseId; using firebase::firestore::model::DocumentKeySet; using firebase::firestore::model::OnlineState; @@ -191,7 +192,7 @@ - (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistenc // TODO(gsoltis): enable LRU GC once API is finalized FSTLevelDB *ldb = [[FSTLevelDB alloc] initWithDirectory:std::move(dir) serializer:serializer - lruGcParams:FSTLruGcParams::Disabled()]; + lruParams:LruParams::Disabled()]; _lruDelegate = ldb.referenceDelegate; _persistence = ldb; [self scheduleLruGarbageCollection]; @@ -244,7 +245,7 @@ - (void)scheduleLruGarbageCollection { long delay = _gcHasRun ? _initialGcDelay : _regularGcDelay; _lruCallback = [_workerDispatchQueue dispatchAfterDelay:delay - timerID:FSTLruGCTimer + timerID:FSTTimerIDGarbageCollectionDelay block:^{ FSTLruGcResults results = [self->_localStore tryLruGarbageCollection:self->_lruDelegate.gc]; diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index 473159b5824..c9cb29480b4 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -29,15 +29,17 @@ extern const firebase::firestore::model::ListenSequenceNumber kFSTListenSequenceNumberInvalid; -struct FSTLruGcParams { - static FSTLruGcParams Default() { - return FSTLruGcParams{.minBytesThreshold = 100 * 1024 * 1024, - .percentileToCollect = 10, - .maximumSequenceNumbersToCollect = 1000}; +namespace firebase { +namespace firestore { +namespace local { + +struct LruParams { + static LruParams Default() { + return LruParams{100 * 1024 * 1024, 10, 1000}; } - static FSTLruGcParams Disabled() { - return FSTLruGcParams{.minBytesThreshold = kFIRFirestorePersistenceCacheSizeUnlimited}; + static LruParams Disabled() { + return LruParams{kFIRFirestorePersistenceCacheSizeUnlimited, 0, 0}; } long minBytesThreshold; @@ -45,6 +47,10 @@ struct FSTLruGcParams { int maximumSequenceNumbersToCollect; }; +} // namespace local +} // namespace firestore +} // namespace firebase + /** * Persistence layers intending to use LRU Garbage collection should implement this protocol. This * protocol defines the operations that the LRU garbage collector needs from the persistence layer. @@ -127,7 +133,8 @@ struct FSTLruGcResults { */ @interface FSTLRUGarbageCollector : NSObject -- (instancetype)initWithDelegate:(id)delegate params:(FSTLruGcParams)params; +- (instancetype)initWithDelegate:(id)delegate + params:(firebase::firestore::local::LruParams)params; /** * Given a target percentile, return the number of queries that make up that percentage of the diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 962c65b4196..d668954a372 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -27,6 +27,7 @@ #include "Firestore/core/src/firebase/firestore/util/log.h" using firebase::Timestamp; +using firebase::firestore::local::LruParams; using firebase::firestore::model::DocumentKey; using firebase::firestore::model::ListenSequenceNumber; @@ -100,12 +101,12 @@ size_t size() const { @implementation FSTLRUGarbageCollector { id _delegate; - FSTLruGcParams _params; + LruParams _params; long _startTime; long _lastRunTime; } -- (instancetype)initWithDelegate:(id)delegate params:(FSTLruGcParams)params { +- (instancetype)initWithDelegate:(id)delegate params:(LruParams)params { self = [super init]; if (self) { _delegate = delegate; diff --git a/Firestore/Source/Local/FSTLevelDB.h b/Firestore/Source/Local/FSTLevelDB.h index 1cc04c672e3..3128587a463 100644 --- a/Firestore/Source/Local/FSTLevelDB.h +++ b/Firestore/Source/Local/FSTLevelDB.h @@ -45,7 +45,8 @@ NS_ASSUME_NONNULL_BEGIN */ - (instancetype)initWithDirectory:(firebase::firestore::util::Path)directory serializer:(FSTLocalSerializer *)serializer - lruGcParams:(FSTLruGcParams)lruGcParams NS_DESIGNATED_INITIALIZER; + lruParams:(firebase::firestore::local::LruParams)lruParams + NS_DESIGNATED_INITIALIZER; - (instancetype)init NS_UNAVAILABLE; diff --git a/Firestore/Source/Local/FSTLevelDB.mm b/Firestore/Source/Local/FSTLevelDB.mm index fc5791f95cc..5cc33f68c94 100644 --- a/Firestore/Source/Local/FSTLevelDB.mm +++ b/Firestore/Source/Local/FSTLevelDB.mm @@ -61,6 +61,7 @@ using firebase::firestore::local::LevelDbMigrations; using firebase::firestore::local::LevelDbMutationKey; using firebase::firestore::local::LevelDbTransaction; +using firebase::firestore::local::LruParams; using firebase::firestore::model::DatabaseId; using firebase::firestore::model::DocumentKey; using firebase::firestore::model::ListenSequenceNumber; @@ -112,10 +113,9 @@ @implementation FSTLevelDBLRUDelegate { FSTListenSequence *_listenSequence; } -- (instancetype)initWithPersistence:(FSTLevelDB *)persistence - lruGcParams:(FSTLruGcParams)lruGcParams { +- (instancetype)initWithPersistence:(FSTLevelDB *)persistence lruParams:(LruParams)lruParams { if (self = [super init]) { - _gc = [[FSTLRUGarbageCollector alloc] initWithDelegate:self params:lruGcParams]; + _gc = [[FSTLRUGarbageCollector alloc] initWithDelegate:self params:lruParams]; _db = persistence; _currentSequenceNumber = kFSTListenSequenceNumberInvalid; } @@ -294,15 +294,15 @@ + (const ReadOptions)standardReadOptions { return users; } -- (instancetype)initWithDirectory:(Path)directory +- (instancetype)initWithDirectory:(firebase::firestore::util::Path)directory serializer:(FSTLocalSerializer *)serializer - lruGcParams:(FSTLruGcParams)lruGcParams { + lruParams:(firebase::firestore::local::LruParams)lruParams { if (self = [super init]) { _directory = std::move(directory); _serializer = serializer; _queryCache = [[FSTLevelDBQueryCache alloc] initWithDB:self serializer:self.serializer]; _referenceDelegate = - [[FSTLevelDBLRUDelegate alloc] initWithPersistence:self lruGcParams:lruGcParams]; + [[FSTLevelDBLRUDelegate alloc] initWithPersistence:self lruParams:lruParams]; _transactionRunner.SetBackingPersistence(self); } return self; diff --git a/Firestore/Source/Local/FSTMemoryPersistence.h b/Firestore/Source/Local/FSTMemoryPersistence.h index 1c535d185ef..c2938435dfa 100644 --- a/Firestore/Source/Local/FSTMemoryPersistence.h +++ b/Firestore/Source/Local/FSTMemoryPersistence.h @@ -32,8 +32,8 @@ NS_ASSUME_NONNULL_BEGIN + (instancetype)persistenceWithEagerGC; -+ (instancetype)persistenceWithLruGcParams:(FSTLruGcParams)lruGcParams - serializer:(FSTLocalSerializer *)serializer; ++ (instancetype)persistenceWithLruParams:(firebase::firestore::local::LruParams)lruParams + serializer:(FSTLocalSerializer *)serializer; @end @@ -54,7 +54,7 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)initWithPersistence:(FSTMemoryPersistence *)persistence serializer:(FSTLocalSerializer *)serializer - lruGcParams:(FSTLruGcParams)lruGcParams; + lruParams:(firebase::firestore::local::LruParams)lruParams; - (BOOL)isPinnedAtSequenceNumber:(firebase::firestore::model::ListenSequenceNumber)upperBound document:(const firebase::firestore::model::DocumentKey &)key; diff --git a/Firestore/Source/Local/FSTMemoryPersistence.mm b/Firestore/Source/Local/FSTMemoryPersistence.mm index 656ff7bdac8..905ca5dc91c 100644 --- a/Firestore/Source/Local/FSTMemoryPersistence.mm +++ b/Firestore/Source/Local/FSTMemoryPersistence.mm @@ -33,6 +33,7 @@ using firebase::firestore::auth::HashUser; using firebase::firestore::auth::User; +using firebase::firestore::local::LruParams; using firebase::firestore::model::DocumentKey; using firebase::firestore::model::DocumentKeyHash; using firebase::firestore::model::ListenSequenceNumber; @@ -83,13 +84,13 @@ + (instancetype)persistenceWithEagerGC { return persistence; } -+ (instancetype)persistenceWithLruGcParams:(FSTLruGcParams)lruGcParams - serializer:(FSTLocalSerializer *)serializer { ++ (instancetype)persistenceWithLruParams:(firebase::firestore::local::LruParams)lruParams + serializer:(FSTLocalSerializer *)serializer { FSTMemoryPersistence *persistence = [[FSTMemoryPersistence alloc] init]; persistence.referenceDelegate = [[FSTMemoryLRUReferenceDelegate alloc] initWithPersistence:persistence serializer:serializer - lruGcParams:lruGcParams]; + lruParams:lruParams]; return persistence; } @@ -167,10 +168,10 @@ @implementation FSTMemoryLRUReferenceDelegate { - (instancetype)initWithPersistence:(FSTMemoryPersistence *)persistence serializer:(FSTLocalSerializer *)serializer - lruGcParams:(FSTLruGcParams)lruGcParams { + lruParams:(firebase::firestore::local::LruParams)lruParams { if (self = [super init]) { _persistence = persistence; - _gc = [[FSTLRUGarbageCollector alloc] initWithDelegate:self params:lruGcParams]; + _gc = [[FSTLRUGarbageCollector alloc] initWithDelegate:self params:lruParams]; _currentSequenceNumber = kFSTListenSequenceNumberInvalid; // Theoretically this is always 0, since this is all in-memory... ListenSequenceNumber highestSequenceNumber = diff --git a/Firestore/Source/Util/FSTDispatchQueue.h b/Firestore/Source/Util/FSTDispatchQueue.h index 8c550a29f4f..75fe4a2f3df 100644 --- a/Firestore/Source/Util/FSTDispatchQueue.h +++ b/Firestore/Source/Util/FSTDispatchQueue.h @@ -46,7 +46,7 @@ typedef NS_ENUM(NSInteger, FSTTimerID) { /** * A timer used to periodically attempt LRU Garbage collection */ - FSTLruGCTimer + FSTTimerIDGarbageCollectionDelay }; /** From 95e6f77dbeee249225066ed4e81a62010ae305ab Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Fri, 5 Oct 2018 09:22:40 -0700 Subject: [PATCH 05/33] add NS tags to initializers, switch gc call to collect --- .../Example/Tests/Local/FSTLRUGarbageCollectorTests.mm | 6 +++--- Firestore/Source/Local/FSTLRUGarbageCollector.h | 6 ++++-- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 2 +- Firestore/Source/Local/FSTLocalStore.mm | 2 +- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm index b8381ff9138..aec82f914b6 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm @@ -685,7 +685,7 @@ - (void)testDisabled { } }); - FSTLruGcResults results = [_gc tryRunGcWithLiveTargets:@{}]; + FSTLruGcResults results = [_gc collectWithLiveTargets:@{}]; XCTAssertFalse(results.didRun); [_persistence shutdown]; @@ -710,7 +710,7 @@ - (void)testCacheTooSmall { XCTAssertLessThan(cacheSize, params.minBytesThreshold); // Try collection and verify that it didn't run - FSTLruGcResults results = [_gc tryRunGcWithLiveTargets:@{}]; + FSTLruGcResults results = [_gc collectWithLiveTargets:@{}]; XCTAssertFalse(results.didRun); [_persistence shutdown]; @@ -739,7 +739,7 @@ - (void)testGCRan { // Mark nothing as live, so everything is eligible. FSTLruGcResults results = _persistence.run( - "GC", [&]() -> FSTLruGcResults { return [_gc tryRunGcWithLiveTargets:@{}]; }); + "GC", [&]() -> FSTLruGcResults { return [_gc collectWithLiveTargets:@{}]; }); // By default, we collect 10% of the sequence numbers. Since we added 100 targets, // that should be 10 targets with 10 documents each, for a total of 100 documents. diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index c9cb29480b4..d41979f9d36 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -134,7 +134,9 @@ struct FSTLruGcResults { @interface FSTLRUGarbageCollector : NSObject - (instancetype)initWithDelegate:(id)delegate - params:(firebase::firestore::local::LruParams)params; + params:(firebase::firestore::local::LruParams)params NS_DESIGNATED_INITIALIZER; + +- (instancetype)init NS_UNAVAILABLE; /** * Given a target percentile, return the number of queries that make up that percentage of the @@ -166,6 +168,6 @@ struct FSTLruGcResults { - (size_t)byteSize; -- (FSTLruGcResults)tryRunGcWithLiveTargets:(NSDictionary *)liveTargets; +- (FSTLruGcResults)collectWithLiveTargets:(NSDictionary *)liveTargets; @end diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index d668954a372..4a474d73ad4 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -116,7 +116,7 @@ - (instancetype)initWithDelegate:(id)delegate params:(LruParams) return self; } -- (FSTLruGcResults)tryRunGcWithLiveTargets:(NSDictionary *)liveTargets { +- (FSTLruGcResults)collectWithLiveTargets:(NSDictionary *)liveTargets { if (_params.minBytesThreshold == kFIRFirestorePersistenceCacheSizeUnlimited) { return FSTLruGcResults::DidNotRun(); } diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index ffbdeb3a48f..05e4acc939d 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -565,7 +565,7 @@ - (void)applyBatchResult:(FSTMutationBatchResult *)batchResult { } - (FSTLruGcResults)tryLruGarbageCollection:(FSTLRUGarbageCollector *)garbageCollector { - return [garbageCollector tryRunGcWithLiveTargets:_targetIDs]; + return [garbageCollector collectWithLiveTargets:_targetIDs]; } @end From 42e15665be9ed7c73a214b03f765626a3667ddf4 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Fri, 5 Oct 2018 10:29:07 -0700 Subject: [PATCH 06/33] Remove timing from lru results struct --- Firestore/Source/Core/FSTFirestoreClient.mm | 4 +-- .../Source/Local/FSTLRUGarbageCollector.h | 12 ------- .../Source/Local/FSTLRUGarbageCollector.mm | 32 ++++++++++++------- 3 files changed, 22 insertions(+), 26 deletions(-) diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index aab245ed436..6718fbd53ca 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -247,10 +247,8 @@ - (void)scheduleLruGarbageCollection { dispatchAfterDelay:delay timerID:FSTTimerIDGarbageCollectionDelay block:^{ - FSTLruGcResults results = - [self->_localStore tryLruGarbageCollection:self->_lruDelegate.gc]; + [self->_localStore tryLruGarbageCollection:self->_lruDelegate.gc]; self->_gcHasRun = YES; - LOG_DEBUG(results.ToString().c_str()); [self scheduleLruGarbageCollection]; }]; } diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index d41979f9d36..e9aaa1a38da 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -109,19 +109,7 @@ struct FSTLruGcResults { std::string ToString() const; - long total_duration() const { - return targetCountDurationMs + upperBoundDurationMs + removedTargetsDurationMs + - removedDocumentsDurationMs + dbCompactionDurationMs; - } - BOOL didRun; - - long targetCountDurationMs; - long upperBoundDurationMs; - long removedTargetsDurationMs; - long removedDocumentsDurationMs; - long dbCompactionDurationMs; - int sequenceNumbersCollected; int targetsRemoved; int documentsRemoved; diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 4a474d73ad4..fb882c17acd 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -81,7 +81,7 @@ size_t size() const { const size_t max_elements_; }; -std::string FSTLruGcResults::ToString() const { +/*std::string FSTLruGcResults::ToString() const { if (!didRun) { return "Garbage Collection skipped"; } else { @@ -97,7 +97,7 @@ size_t size() const { absl::StrAppend(&desc, "Total duration: ", total_duration(), "ms"); return desc; } -} +}*/ @implementation FSTLRUGarbageCollector { id _delegate; @@ -118,12 +118,14 @@ - (instancetype)initWithDelegate:(id)delegate params:(LruParams) - (FSTLruGcResults)collectWithLiveTargets:(NSDictionary *)liveTargets { if (_params.minBytesThreshold == kFIRFirestorePersistenceCacheSizeUnlimited) { + LOG_DEBUG("Garbage collection skipped; disabled"); return FSTLruGcResults::DidNotRun(); } size_t currentSize = [self byteSize]; if (currentSize < _params.minBytesThreshold) { // Not enough on disk to warrant collection. Wait another timeout cycle. + LOG_DEBUG("Garbage collection skipped; Cache size %i is lower than threshold %i", currentSize, _params.minBytesThreshold); return FSTLruGcResults::DidNotRun(); } else { return [self runGCWithLiveTargets:liveTargets]; @@ -152,16 +154,24 @@ - (FSTLruGcResults)runGCWithLiveTargets:(NSDictionary Date: Fri, 5 Oct 2018 10:54:35 -0700 Subject: [PATCH 07/33] Switch to chrono for times --- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index fb882c17acd..fd22d0059a2 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -16,6 +16,7 @@ #import "Firestore/Source/Local/FSTLRUGarbageCollector.h" +#include #include #include @@ -26,6 +27,7 @@ #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/util/log.h" +using Millis = std::chrono::milliseconds; using firebase::Timestamp; using firebase::firestore::local::LruParams; using firebase::firestore::model::DocumentKey; @@ -34,12 +36,9 @@ const long kFIRFirestorePersistenceCacheSizeUnlimited = -1; const ListenSequenceNumber kFSTListenSequenceNumberInvalid = -1; -static long toMilliseconds(const Timestamp &ts) { - return (1000 * ts.seconds()) + (ts.nanoseconds() / 1000000); -} - -static long millisecondsBetween(const Timestamp &start, const Timestamp &end) { - return toMilliseconds(end) - toMilliseconds(start); +static Millis::rep millisecondsBetween(const Timestamp &start, const Timestamp &end) { + //return toMilliseconds(end) - toMilliseconds(start); + return std::chrono::duration_cast(end.ToTimePoint() - start.ToTimePoint()).count(); } /** @@ -154,7 +153,6 @@ - (FSTLruGcResults)runGCWithLiveTargets:(NSDictionary Date: Fri, 5 Oct 2018 11:20:31 -0700 Subject: [PATCH 08/33] Move LruResults in C++ namespaces --- .../Local/FSTLRUGarbageCollectorTests.mm | 9 +++--- .../Source/Local/FSTLRUGarbageCollector.h | 26 ++++++++--------- .../Source/Local/FSTLRUGarbageCollector.mm | 29 ++++--------------- Firestore/Source/Local/FSTLocalStore.h | 5 ++-- Firestore/Source/Local/FSTLocalStore.mm | 3 +- 5 files changed, 27 insertions(+), 45 deletions(-) diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm index aec82f914b6..5415a1add56 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm @@ -41,6 +41,7 @@ namespace testutil = firebase::firestore::testutil; using firebase::firestore::auth::User; using firebase::firestore::local::LruParams; +using firebase::firestore::local::LruResults; using firebase::firestore::model::DocumentKey; using firebase::firestore::model::DocumentKeyHash; using firebase::firestore::model::DocumentKeySet; @@ -685,7 +686,7 @@ - (void)testDisabled { } }); - FSTLruGcResults results = [_gc collectWithLiveTargets:@{}]; + LruResults results = [_gc collectWithLiveTargets:@{}]; XCTAssertFalse(results.didRun); [_persistence shutdown]; @@ -710,7 +711,7 @@ - (void)testCacheTooSmall { XCTAssertLessThan(cacheSize, params.minBytesThreshold); // Try collection and verify that it didn't run - FSTLruGcResults results = [_gc collectWithLiveTargets:@{}]; + LruResults results = [_gc collectWithLiveTargets:@{}]; XCTAssertFalse(results.didRun); [_persistence shutdown]; @@ -738,8 +739,8 @@ - (void)testGCRan { } // Mark nothing as live, so everything is eligible. - FSTLruGcResults results = _persistence.run( - "GC", [&]() -> FSTLruGcResults { return [_gc collectWithLiveTargets:@{}]; }); + LruResults results = _persistence.run( + "GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; }); // By default, we collect 10% of the sequence numbers. Since we added 100 targets, // that should be 10 targets with 10 documents each, for a total of 100 documents. diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index e9aaa1a38da..8642e55873e 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -47,6 +47,17 @@ struct LruParams { int maximumSequenceNumbersToCollect; }; +struct LruResults { + static LruResults DidNotRun() { + return LruResults{NO, 0, 0, 0}; + } + + BOOL didRun; + int sequenceNumbersCollected; + int targetsRemoved; + int documentsRemoved; +}; + } // namespace local } // namespace firestore } // namespace firebase @@ -102,19 +113,6 @@ struct LruParams { @end -struct FSTLruGcResults { - static FSTLruGcResults DidNotRun() { - return FSTLruGcResults{.didRun = NO}; - } - - std::string ToString() const; - - BOOL didRun; - int sequenceNumbersCollected; - int targetsRemoved; - int documentsRemoved; -}; - /** * FSTLRUGarbageCollector defines the LRU algorithm used to clean up old documents and targets. It * is persistence-agnostic, as long as proper delegate is provided. @@ -156,6 +154,6 @@ struct FSTLruGcResults { - (size_t)byteSize; -- (FSTLruGcResults)collectWithLiveTargets:(NSDictionary *)liveTargets; +- (firebase::firestore::local::LruResults)collectWithLiveTargets:(NSDictionary *)liveTargets; @end diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index fd22d0059a2..6229f2cb949 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -30,6 +30,7 @@ using Millis = std::chrono::milliseconds; using firebase::Timestamp; using firebase::firestore::local::LruParams; +using firebase::firestore::local::LruResults; using firebase::firestore::model::DocumentKey; using firebase::firestore::model::ListenSequenceNumber; @@ -80,24 +81,6 @@ size_t size() const { const size_t max_elements_; }; -/*std::string FSTLruGcResults::ToString() const { - if (!didRun) { - return "Garbage Collection skipped"; - } else { - std::string desc = "LRU Garbage Collection:\n"; - absl::StrAppend(&desc, "\tCounted targets in ", targetCountDurationMs, "ms\n"); - absl::StrAppend(&desc, "\tDetermined least recently used ", sequenceNumbersCollected, - " sequence numbers in ", upperBoundDurationMs, "ms\n"); - absl::StrAppend(&desc, "\tRemoved ", targetsRemoved, " targets in ", removedTargetsDurationMs, - "ms\n"); - absl::StrAppend(&desc, "\tRemoved ", documentsRemoved, " documents in ", - removedDocumentsDurationMs, "ms\n"); - absl::StrAppend(&desc, "\tCompacted leveldb database in ", dbCompactionDurationMs, "ms\n"); - absl::StrAppend(&desc, "Total duration: ", total_duration(), "ms"); - return desc; - } -}*/ - @implementation FSTLRUGarbageCollector { id _delegate; LruParams _params; @@ -115,23 +98,23 @@ - (instancetype)initWithDelegate:(id)delegate params:(LruParams) return self; } -- (FSTLruGcResults)collectWithLiveTargets:(NSDictionary *)liveTargets { +- (LruResults)collectWithLiveTargets:(NSDictionary *)liveTargets { if (_params.minBytesThreshold == kFIRFirestorePersistenceCacheSizeUnlimited) { LOG_DEBUG("Garbage collection skipped; disabled"); - return FSTLruGcResults::DidNotRun(); + return LruResults::DidNotRun(); } size_t currentSize = [self byteSize]; if (currentSize < _params.minBytesThreshold) { // Not enough on disk to warrant collection. Wait another timeout cycle. LOG_DEBUG("Garbage collection skipped; Cache size %i is lower than threshold %i", currentSize, _params.minBytesThreshold); - return FSTLruGcResults::DidNotRun(); + return LruResults::DidNotRun(); } else { return [self runGCWithLiveTargets:liveTargets]; } } -- (FSTLruGcResults)runGCWithLiveTargets:(NSDictionary *)liveTargets { +- (LruResults)runGCWithLiveTargets:(NSDictionary *)liveTargets { Timestamp start = Timestamp::Now(); int sequenceNumbers = [self queryCountForPercentile:_params.percentileToCollect]; // Cap at the configured max @@ -165,7 +148,7 @@ - (FSTLruGcResults)runGCWithLiveTargets:(NSDictionary +#import "Firestore/Source/Local/FSTLRUGarbageCollector.h" #import "Firestore/Source/Model/FSTDocumentDictionary.h" #include "Firestore/core/src/firebase/firestore/auth/user.h" @@ -26,8 +27,6 @@ @class FSTLocalViewChanges; @class FSTLocalWriteResult; -@class FSTLRUGarbageCollector; -struct FSTLruGcResults; @class FSTMutation; @class FSTMutationBatch; @class FSTMutationBatchResult; @@ -178,7 +177,7 @@ NS_ASSUME_NONNULL_BEGIN - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID: (firebase::firestore::model::BatchId)batchID; -- (FSTLruGcResults)tryLruGarbageCollection:(FSTLRUGarbageCollector *)garbageCollector; +- (firebase::firestore::local::LruResults)tryLruGarbageCollection:(FSTLRUGarbageCollector *)garbageCollector; @end diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index 05e4acc939d..c501f47f6d3 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -46,6 +46,7 @@ using firebase::firestore::auth::User; using firebase::firestore::core::TargetIdGenerator; +using firebase::firestore::local::LruResults; using firebase::firestore::model::BatchId; using firebase::firestore::model::DocumentKey; using firebase::firestore::model::DocumentKeySet; @@ -564,7 +565,7 @@ - (void)applyBatchResult:(FSTMutationBatchResult *)batchResult { } } -- (FSTLruGcResults)tryLruGarbageCollection:(FSTLRUGarbageCollector *)garbageCollector { +- (LruResults)tryLruGarbageCollection:(FSTLRUGarbageCollector *)garbageCollector { return [garbageCollector collectWithLiveTargets:_targetIDs]; } From eaa682f227fd7dceacfb17cef4d48cd29492f6ba Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Fri, 5 Oct 2018 11:27:54 -0700 Subject: [PATCH 09/33] Rename localstore garbage collection method --- Firestore/Source/Core/FSTFirestoreClient.mm | 2 +- Firestore/Source/Local/FSTLocalStore.h | 2 +- Firestore/Source/Local/FSTLocalStore.mm | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 6718fbd53ca..d360612265f 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -247,7 +247,7 @@ - (void)scheduleLruGarbageCollection { dispatchAfterDelay:delay timerID:FSTTimerIDGarbageCollectionDelay block:^{ - [self->_localStore tryLruGarbageCollection:self->_lruDelegate.gc]; + [self->_localStore collectGarbage:self->_lruDelegate.gc]; self->_gcHasRun = YES; [self scheduleLruGarbageCollection]; }]; diff --git a/Firestore/Source/Local/FSTLocalStore.h b/Firestore/Source/Local/FSTLocalStore.h index 5ec34d43c95..0ea15110503 100644 --- a/Firestore/Source/Local/FSTLocalStore.h +++ b/Firestore/Source/Local/FSTLocalStore.h @@ -177,7 +177,7 @@ NS_ASSUME_NONNULL_BEGIN - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID: (firebase::firestore::model::BatchId)batchID; -- (firebase::firestore::local::LruResults)tryLruGarbageCollection:(FSTLRUGarbageCollector *)garbageCollector; +- (firebase::firestore::local::LruResults)collectGarbage:(FSTLRUGarbageCollector *)garbageCollector; @end diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index c501f47f6d3..722227494dc 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -565,7 +565,7 @@ - (void)applyBatchResult:(FSTMutationBatchResult *)batchResult { } } -- (LruResults)tryLruGarbageCollection:(FSTLRUGarbageCollector *)garbageCollector { +- (LruResults)collectGarbage:(FSTLRUGarbageCollector *)garbageCollector { return [garbageCollector collectWithLiveTargets:_targetIDs]; } From ee2cd938c0131ff9ba9dbfaf38080cf31a101fb4 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Fri, 5 Oct 2018 11:28:30 -0700 Subject: [PATCH 10/33] Style --- .../Local/FSTLRUGarbageCollectorTests.mm | 4 +-- Firestore/Source/Core/FSTFirestoreClient.mm | 16 ++++++------ .../Source/Local/FSTLRUGarbageCollector.h | 6 +++-- .../Source/Local/FSTLRUGarbageCollector.mm | 25 +++++++++---------- 4 files changed, 26 insertions(+), 25 deletions(-) diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm index 5415a1add56..aa8da86b2cb 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm @@ -739,8 +739,8 @@ - (void)testGCRan { } // Mark nothing as live, so everything is eligible. - LruResults results = _persistence.run( - "GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; }); + LruResults results = + _persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; }); // By default, we collect 10% of the sequence numbers. Since we added 100 targets, // that should be 10 targets with 10 documents each, for a total of 100 documents. diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index d360612265f..60b2b199e70 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -243,14 +243,14 @@ - (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistenc */ - (void)scheduleLruGarbageCollection { long delay = _gcHasRun ? _initialGcDelay : _regularGcDelay; - _lruCallback = [_workerDispatchQueue - dispatchAfterDelay:delay - timerID:FSTTimerIDGarbageCollectionDelay - block:^{ - [self->_localStore collectGarbage:self->_lruDelegate.gc]; - self->_gcHasRun = YES; - [self scheduleLruGarbageCollection]; - }]; + _lruCallback = + [_workerDispatchQueue dispatchAfterDelay:delay + timerID:FSTTimerIDGarbageCollectionDelay + block:^{ + [self->_localStore collectGarbage:self->_lruDelegate.gc]; + self->_gcHasRun = YES; + [self scheduleLruGarbageCollection]; + }]; } - (void)credentialDidChangeWithUser:(const User &)user { diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index 8642e55873e..a6678ff7310 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -120,7 +120,8 @@ struct LruResults { @interface FSTLRUGarbageCollector : NSObject - (instancetype)initWithDelegate:(id)delegate - params:(firebase::firestore::local::LruParams)params NS_DESIGNATED_INITIALIZER; + params:(firebase::firestore::local::LruParams)params + NS_DESIGNATED_INITIALIZER; - (instancetype)init NS_UNAVAILABLE; @@ -154,6 +155,7 @@ struct LruResults { - (size_t)byteSize; -- (firebase::firestore::local::LruResults)collectWithLiveTargets:(NSDictionary *)liveTargets; +- (firebase::firestore::local::LruResults)collectWithLiveTargets: + (NSDictionary *)liveTargets; @end diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 6229f2cb949..6f1e6138ce0 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -38,7 +38,6 @@ const ListenSequenceNumber kFSTListenSequenceNumberInvalid = -1; static Millis::rep millisecondsBetween(const Timestamp &start, const Timestamp &end) { - //return toMilliseconds(end) - toMilliseconds(start); return std::chrono::duration_cast(end.ToTimePoint() - start.ToTimePoint()).count(); } @@ -107,7 +106,8 @@ - (LruResults)collectWithLiveTargets:(NSDictionary * size_t currentSize = [self byteSize]; if (currentSize < _params.minBytesThreshold) { // Not enough on disk to warrant collection. Wait another timeout cycle. - LOG_DEBUG("Garbage collection skipped; Cache size %i is lower than threshold %i", currentSize, _params.minBytesThreshold); + LOG_DEBUG("Garbage collection skipped; Cache size %i is lower than threshold %i", currentSize, + _params.minBytesThreshold); return LruResults::DidNotRun(); } else { return [self runGCWithLiveTargets:liveTargets]; @@ -137,22 +137,21 @@ - (LruResults)runGCWithLiveTargets:(NSDictionary *)l Timestamp compactedDb = Timestamp::Now(); std::string desc = "LRU Garbage Collection:\n"; - absl::StrAppend(&desc, "\tCounted targets in ", millisecondsBetween(start, countedTargets), "ms\n"); + absl::StrAppend(&desc, "\tCounted targets in ", millisecondsBetween(start, countedTargets), + "ms\n"); absl::StrAppend(&desc, "\tDetermined least recently used ", sequenceNumbers, - " sequence numbers in ", millisecondsBetween(countedTargets, foundUpperBound), "ms\n"); - absl::StrAppend(&desc, "\tRemoved ", numTargetsRemoved, " targets in ", millisecondsBetween(foundUpperBound, removedTargets), - "ms\n"); + " sequence numbers in ", millisecondsBetween(countedTargets, foundUpperBound), + "ms\n"); + absl::StrAppend(&desc, "\tRemoved ", numTargetsRemoved, " targets in ", + millisecondsBetween(foundUpperBound, removedTargets), "ms\n"); absl::StrAppend(&desc, "\tRemoved ", numDocumentsRemoved, " documents in ", - millisecondsBetween(removedTargets, removedDocuments), "ms\n"); - absl::StrAppend(&desc, "\tCompacted leveldb database in ", millisecondsBetween(removedDocuments, compactedDb), "ms\n"); + millisecondsBetween(removedTargets, removedDocuments), "ms\n"); + absl::StrAppend(&desc, "\tCompacted leveldb database in ", + millisecondsBetween(removedDocuments, compactedDb), "ms\n"); absl::StrAppend(&desc, "Total duration: ", millisecondsBetween(start, compactedDb), "ms"); LOG_DEBUG(desc.c_str()); - return LruResults{ - YES, - sequenceNumbers, - numTargetsRemoved, - numDocumentsRemoved}; + return LruResults{YES, sequenceNumbers, numTargetsRemoved, numDocumentsRemoved}; } - (int)queryCountForPercentile:(NSUInteger)percentile { From 6b707ba7763f07b99aa52b3b0042de7c2ddc93d4 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Fri, 5 Oct 2018 11:43:06 -0700 Subject: [PATCH 11/33] exempt chrono from linting --- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 6f1e6138ce0..81f5864c121 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -16,7 +16,7 @@ #import "Firestore/Source/Local/FSTLRUGarbageCollector.h" -#include +#include //NOLINT(build/c++11) #include #include From dc948b9ba604e2869a3d2772afce00271c5b2bbd Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Fri, 5 Oct 2018 12:09:01 -0700 Subject: [PATCH 12/33] Style --- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 81f5864c121..83e41071bf4 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -16,7 +16,7 @@ #import "Firestore/Source/Local/FSTLRUGarbageCollector.h" -#include //NOLINT(build/c++11) +#include //NOLINT(build/c++11) #include #include From 9aec5745ea5b70020f849e7f9ec8b67e68ed2752 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Fri, 5 Oct 2018 12:40:51 -0700 Subject: [PATCH 13/33] BOOL -> bool --- Firestore/Source/Local/FSTLRUGarbageCollector.h | 4 ++-- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index a6678ff7310..f6b2806540a 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -49,10 +49,10 @@ struct LruParams { struct LruResults { static LruResults DidNotRun() { - return LruResults{NO, 0, 0, 0}; + return LruResults{/* didRun= */ false, 0, 0, 0}; } - BOOL didRun; + bool didRun; int sequenceNumbersCollected; int targetsRemoved; int documentsRemoved; diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 83e41071bf4..d582ca581bc 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -151,7 +151,7 @@ - (LruResults)runGCWithLiveTargets:(NSDictionary *)l absl::StrAppend(&desc, "Total duration: ", millisecondsBetween(start, compactedDb), "ms"); LOG_DEBUG(desc.c_str()); - return LruResults{YES, sequenceNumbers, numTargetsRemoved, numDocumentsRemoved}; + return LruResults{/* didRun= */ true, sequenceNumbers, numTargetsRemoved, numDocumentsRemoved}; } - (int)queryCountForPercentile:(NSUInteger)percentile { From be136d322a3dc20c8a1e6fb64e02372faf4bdcaa Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Fri, 5 Oct 2018 14:51:19 -0700 Subject: [PATCH 14/33] Define unlimited cache size for ObjC --- Firestore/Source/Local/FSTLRUGarbageCollector.h | 4 +++- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 4 ++-- Firestore/Source/Public/FIRFirestoreSettings.h | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index f6b2806540a..61251b706d7 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -34,12 +34,14 @@ namespace firestore { namespace local { struct LruParams { + static const int64_t CacheSizeUnlimited = -1; + static LruParams Default() { return LruParams{100 * 1024 * 1024, 10, 1000}; } static LruParams Disabled() { - return LruParams{kFIRFirestorePersistenceCacheSizeUnlimited, 0, 0}; + return LruParams{kFIRFirestoreCacheSizeUnlimited, 0, 0}; } long minBytesThreshold; diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index d582ca581bc..3d5edffeb5c 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -34,7 +34,7 @@ using firebase::firestore::model::DocumentKey; using firebase::firestore::model::ListenSequenceNumber; -const long kFIRFirestorePersistenceCacheSizeUnlimited = -1; +const int64_t kFIRFirestoreCacheSizeUnlimited = LruParams::CacheSizeUnlimited; const ListenSequenceNumber kFSTListenSequenceNumberInvalid = -1; static Millis::rep millisecondsBetween(const Timestamp &start, const Timestamp &end) { @@ -98,7 +98,7 @@ - (instancetype)initWithDelegate:(id)delegate params:(LruParams) } - (LruResults)collectWithLiveTargets:(NSDictionary *)liveTargets { - if (_params.minBytesThreshold == kFIRFirestorePersistenceCacheSizeUnlimited) { + if (_params.minBytesThreshold == kFIRFirestoreCacheSizeUnlimited) { LOG_DEBUG("Garbage collection skipped; disabled"); return LruResults::DidNotRun(); } diff --git a/Firestore/Source/Public/FIRFirestoreSettings.h b/Firestore/Source/Public/FIRFirestoreSettings.h index 49a4282ab30..919aadbab76 100644 --- a/Firestore/Source/Public/FIRFirestoreSettings.h +++ b/Firestore/Source/Public/FIRFirestoreSettings.h @@ -19,7 +19,7 @@ NS_ASSUME_NONNULL_BEGIN /** Used to set on-disk cache size to unlimited. Garbage collection will not run. */ -extern const long kFIRFirestorePersistenceCacheSizeUnlimited; +extern const int64_t kFIRFirestoreCacheSizeUnlimited; /** Settings used to configure a `FIRFirestore` instance. */ NS_SWIFT_NAME(FirestoreSettings) From 618f329ea99481a51da814f6d74e2bc4750c437e Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Fri, 5 Oct 2018 15:54:58 -0700 Subject: [PATCH 15/33] Fix type --- Firestore/Source/Local/FSTLRUGarbageCollector.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index 61251b706d7..3258cf13556 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -44,7 +44,7 @@ struct LruParams { return LruParams{kFIRFirestoreCacheSizeUnlimited, 0, 0}; } - long minBytesThreshold; + int64_t minBytesThreshold; int percentileToCollect; int maximumSequenceNumbersToCollect; }; From 161df33da61fdb660b83ca9496ee7b54809dca30 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Mon, 8 Oct 2018 09:50:24 -0700 Subject: [PATCH 16/33] Drop unused fields --- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 3 --- 1 file changed, 3 deletions(-) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 3d5edffeb5c..67f8a7aee0f 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -83,8 +83,6 @@ size_t size() const { @implementation FSTLRUGarbageCollector { id _delegate; LruParams _params; - long _startTime; - long _lastRunTime; } - (instancetype)initWithDelegate:(id)delegate params:(LruParams)params { @@ -92,7 +90,6 @@ - (instancetype)initWithDelegate:(id)delegate params:(LruParams) if (self) { _delegate = delegate; _params = std::move(params); - _lastRunTime = -1; } return self; } From e5259ad70b6bc50b096c85fff62e9115b941ca65 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Mon, 8 Oct 2018 10:46:23 -0700 Subject: [PATCH 17/33] Technically calls to GC should be in a transaction, the tests passed because they were testing that GC didn't run --- .../Example/Tests/Local/FSTLRUGarbageCollectorTests.mm | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm index 2a55de7c7a3..8228492260f 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm @@ -694,7 +694,8 @@ - (void)testDisabled { } }); - LruResults results = [_gc collectWithLiveTargets:@{}]; + LruResults results = + _persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; }); XCTAssertFalse(results.didRun); [_persistence shutdown]; @@ -719,7 +720,8 @@ - (void)testCacheTooSmall { XCTAssertLessThan(cacheSize, params.minBytesThreshold); // Try collection and verify that it didn't run - LruResults results = [_gc collectWithLiveTargets:@{}]; + LruResults results = + _persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; }); XCTAssertFalse(results.didRun); [_persistence shutdown]; From e0cda8cf2161308343dc231e49a22ea27a3ffeca Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Mon, 8 Oct 2018 10:47:46 -0700 Subject: [PATCH 18/33] Add transaction in local store as well --- Firestore/Source/Local/FSTLocalStore.mm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index 722227494dc..9a607918538 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -566,7 +566,9 @@ - (void)applyBatchResult:(FSTMutationBatchResult *)batchResult { } - (LruResults)collectGarbage:(FSTLRUGarbageCollector *)garbageCollector { - return [garbageCollector collectWithLiveTargets:_targetIDs]; + return self.persistence.run("Collect garbage", [&]() -> LruResults { + return [garbageCollector collectWithLiveTargets:_targetIDs]; + }); } @end From 8622c2bd8cc8fc7e1d2436d3799f5d46e55286e3 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Mon, 8 Oct 2018 11:23:21 -0700 Subject: [PATCH 19/33] Fix subtle unsigned bug in test --- Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm index 8228492260f..a1161bc1a08 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm @@ -704,7 +704,7 @@ - (void)testDisabled { - (void)testCacheTooSmall { if ([self isTestBaseClass]) return; - LruParams params = LruParams::Disabled(); + LruParams params = LruParams::Default(); [self newTestResourcesWithLruParams:params]; _persistence.run("fill cache", [&]() { @@ -715,7 +715,7 @@ - (void)testCacheTooSmall { } }); - size_t cacheSize = [_gc byteSize]; + int cacheSize = (int)[_gc byteSize]; // Verify that we don't have enough in our cache to warrant collection XCTAssertLessThan(cacheSize, params.minBytesThreshold); From 6d13c580d1d64b59eb7783c2875e23b5ff000efc Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Mon, 8 Oct 2018 12:16:04 -0700 Subject: [PATCH 20/33] Switch to serializedSize method --- Firestore/Source/Local/FSTMemoryMutationQueue.mm | 2 +- Firestore/Source/Local/FSTMemoryQueryCache.mm | 2 +- Firestore/Source/Local/FSTMemoryRemoteDocumentCache.mm | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Firestore/Source/Local/FSTMemoryMutationQueue.mm b/Firestore/Source/Local/FSTMemoryMutationQueue.mm index c4a570eaf83..e72422c44f9 100644 --- a/Firestore/Source/Local/FSTMemoryMutationQueue.mm +++ b/Firestore/Source/Local/FSTMemoryMutationQueue.mm @@ -472,7 +472,7 @@ - (NSUInteger)indexOfExistingBatchID:(BatchId)batchID action:(NSString *)action - (size_t)byteSizeWithSerializer:(FSTLocalSerializer *)serializer { size_t count = 0; for (FSTMutationBatch *batch in self.queue) { - count += [[[serializer encodedMutationBatch:batch] data] length]; + count += [[serializer encodedMutationBatch:batch] serializedSize]; }; return count; } diff --git a/Firestore/Source/Local/FSTMemoryQueryCache.mm b/Firestore/Source/Local/FSTMemoryQueryCache.mm index 16d4055d213..44bf33f7458 100644 --- a/Firestore/Source/Local/FSTMemoryQueryCache.mm +++ b/Firestore/Source/Local/FSTMemoryQueryCache.mm @@ -175,7 +175,7 @@ - (size_t)byteSizeWithSerializer:(FSTLocalSerializer *)serializer { __block size_t count = 0; [self.queries enumerateKeysAndObjectsUsingBlock:^(FSTQuery *key, FSTQueryData *queryData, BOOL *stop) { - count += [[[serializer encodedQueryData:queryData] data] length]; + count += [[serializer encodedQueryData:queryData] serializedSize]; }]; return count; } diff --git a/Firestore/Source/Local/FSTMemoryRemoteDocumentCache.mm b/Firestore/Source/Local/FSTMemoryRemoteDocumentCache.mm index 889268822f5..6a6556b575c 100644 --- a/Firestore/Source/Local/FSTMemoryRemoteDocumentCache.mm +++ b/Firestore/Source/Local/FSTMemoryRemoteDocumentCache.mm @@ -115,7 +115,7 @@ - (size_t)byteSizeWithSerializer:(FSTLocalSerializer *)serializer { [self.docs enumerateKeysAndObjectsUsingBlock:^(FSTDocumentKey *key, FSTMaybeDocument *doc, BOOL *stop) { count += FSTDocumentKeyByteSize(key); - count += [[[serializer encodedMaybeDocument:doc] data] length]; + count += [[serializer encodedMaybeDocument:doc] serializedSize]; }]; return count; } From 55a7608652d676530b3a708af54af91f2df6721c Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Mon, 8 Oct 2018 15:46:11 -0700 Subject: [PATCH 21/33] Fix in-memory lru orphan calculation --- Firestore/Source/Local/FSTMemoryPersistence.mm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Firestore/Source/Local/FSTMemoryPersistence.mm b/Firestore/Source/Local/FSTMemoryPersistence.mm index a52a9a83027..ac9606bdd48 100644 --- a/Firestore/Source/Local/FSTMemoryPersistence.mm +++ b/Firestore/Source/Local/FSTMemoryPersistence.mm @@ -230,7 +230,9 @@ - (void)enumerateMutationsUsingBlock: for (const auto &entry : _sequenceNumbers) { ListenSequenceNumber sequenceNumber = entry.second; const DocumentKey &key = entry.first; - if (![_persistence.queryCache containsKey:key]) { + // Pass in the exact sequence number as the upper bound so we know it won't be pinned by being + // too recent. + if (![self isPinnedAtSequenceNumber:sequenceNumber document:key]) { block(key, sequenceNumber, &stop); } } From c3098d6ca92bd7e0ca3a4ff806440cc08b1d550a Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Wed, 10 Oct 2018 08:54:28 -0700 Subject: [PATCH 22/33] Style --- Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm index a1161bc1a08..8fe90358dc4 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm @@ -695,7 +695,7 @@ - (void)testDisabled { }); LruResults results = - _persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; }); + _persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; }); XCTAssertFalse(results.didRun); [_persistence shutdown]; @@ -721,7 +721,7 @@ - (void)testCacheTooSmall { // Try collection and verify that it didn't run LruResults results = - _persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; }); + _persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; }); XCTAssertFalse(results.didRun); [_persistence shutdown]; From 9894353bf20914b31c5ec8d8f1c374057e859f5a Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Thu, 11 Oct 2018 09:04:33 -0700 Subject: [PATCH 23/33] Wire up LRU GC (#1925) * Wire up LRU GC --- Firestore/Source/API/FIRFirestore.mm | 2 +- Firestore/Source/API/FIRFirestoreSettings.mm | 16 +++++++++++++- Firestore/Source/Core/FSTFirestoreClient.h | 3 ++- Firestore/Source/Core/FSTFirestoreClient.mm | 22 ++++++++++--------- .../Source/Local/FSTLRUGarbageCollector.h | 6 +++++ .../Source/Public/FIRFirestoreSettings.h | 11 +++++++++- .../Swift/Tests/API/BasicCompileTests.swift | 1 + 7 files changed, 47 insertions(+), 14 deletions(-) diff --git a/Firestore/Source/API/FIRFirestore.mm b/Firestore/Source/API/FIRFirestore.mm index 3f33f877bb9..0d786cbb4aa 100644 --- a/Firestore/Source/API/FIRFirestore.mm +++ b/Firestore/Source/API/FIRFirestore.mm @@ -258,7 +258,7 @@ const DatabaseInfo database_info(*self.databaseID, util::MakeString(_persistence absl::make_unique(_settings.dispatchQueue); _client = [FSTFirestoreClient clientWithDatabaseInfo:database_info - usePersistence:_settings.persistenceEnabled + settings:_settings credentialsProvider:_credentialsProvider.get() userExecutor:std::move(userExecutor) workerDispatchQueue:_workerDispatchQueue]; diff --git a/Firestore/Source/API/FIRFirestoreSettings.mm b/Firestore/Source/API/FIRFirestoreSettings.mm index 8f998ec2684..a0efb0b7490 100644 --- a/Firestore/Source/API/FIRFirestoreSettings.mm +++ b/Firestore/Source/API/FIRFirestoreSettings.mm @@ -23,6 +23,8 @@ static NSString *const kDefaultHost = @"firestore.googleapis.com"; static const BOOL kDefaultSSLEnabled = YES; static const BOOL kDefaultPersistenceEnabled = YES; +static const int64_t kDefaultCacheSizeBytes = 100 * 1024 * 1024; +static const int64_t kMinimumCacheSizeBytes = 1 * 1024 * 1024; // TODO(b/73820332): flip the default. static const BOOL kDefaultTimestampsInSnapshotsEnabled = NO; @@ -35,6 +37,7 @@ - (instancetype)init { _dispatchQueue = dispatch_get_main_queue(); _persistenceEnabled = kDefaultPersistenceEnabled; _timestampsInSnapshotsEnabled = kDefaultTimestampsInSnapshotsEnabled; + _cacheSizeBytes = kDefaultCacheSizeBytes; } return self; } @@ -51,7 +54,8 @@ - (BOOL)isEqual:(id)other { self.isSSLEnabled == otherSettings.isSSLEnabled && self.dispatchQueue == otherSettings.dispatchQueue && self.isPersistenceEnabled == otherSettings.isPersistenceEnabled && - self.timestampsInSnapshotsEnabled == otherSettings.timestampsInSnapshotsEnabled; + self.timestampsInSnapshotsEnabled == otherSettings.timestampsInSnapshotsEnabled && + self.cacheSizeBytes == otherSettings.cacheSizeBytes; } - (NSUInteger)hash { @@ -60,6 +64,7 @@ - (NSUInteger)hash { // Ignore the dispatchQueue to avoid having to deal with sizeof(dispatch_queue_t). result = 31 * result + (self.isPersistenceEnabled ? 1231 : 1237); result = 31 * result + (self.timestampsInSnapshotsEnabled ? 1231 : 1237); + result = 31 * result + (NSUInteger)self.cacheSizeBytes; return result; } @@ -70,6 +75,7 @@ - (id)copyWithZone:(nullable NSZone *)zone { copy.dispatchQueue = _dispatchQueue; copy.persistenceEnabled = _persistenceEnabled; copy.timestampsInSnapshotsEnabled = _timestampsInSnapshotsEnabled; + copy.cacheSizeBytes = _cacheSizeBytes; return copy; } @@ -93,6 +99,14 @@ - (void)setDispatchQueue:(dispatch_queue_t)dispatchQueue { _dispatchQueue = dispatchQueue; } +- (void)setCacheSizeBytes:(int64_t)cacheSizeBytes { + if (cacheSizeBytes != kFIRFirestoreCacheSizeUnlimited && + cacheSizeBytes < kMinimumCacheSizeBytes) { + FSTThrowInvalidArgument(@"Cache size must be set to at least %i bytes", kMinimumCacheSizeBytes); + } + _cacheSizeBytes = cacheSizeBytes; +} + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Core/FSTFirestoreClient.h b/Firestore/Source/Core/FSTFirestoreClient.h index 0f38e96bb85..7c76e569603 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.h +++ b/Firestore/Source/Core/FSTFirestoreClient.h @@ -29,6 +29,7 @@ @class FIRDocumentReference; @class FIRDocumentSnapshot; +@class FIRFirestoreSettings; @class FIRQuery; @class FIRQuerySnapshot; @class FSTDatabaseID; @@ -56,7 +57,7 @@ NS_ASSUME_NONNULL_BEGIN * All callbacks and events will be triggered on the provided userExecutor. */ + (instancetype)clientWithDatabaseInfo:(const firebase::firestore::core::DatabaseInfo &)databaseInfo - usePersistence:(BOOL)usePersistence + settings:(FIRFirestoreSettings *)settings credentialsProvider:(firebase::firestore::auth::CredentialsProvider *) credentialsProvider // no passing ownership userExecutor: diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 60b2b199e70..7cd6c9879e7 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -21,6 +21,7 @@ #include #import "FIRFirestoreErrors.h" +#import "FIRFirestoreSettings.h" #import "Firestore/Source/API/FIRDocumentReference+Internal.h" #import "Firestore/Source/API/FIRDocumentSnapshot+Internal.h" #import "Firestore/Source/API/FIRQuery+Internal.h" @@ -75,7 +76,7 @@ @interface FSTFirestoreClient () { } - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo - usePersistence:(BOOL)usePersistence + settings:(FIRFirestoreSettings *)settings credentialsProvider: (CredentialsProvider *)credentialsProvider // no passing ownership userExecutor:(std::unique_ptr)userExecutor @@ -115,20 +116,20 @@ - (Executor *)userExecutor { } + (instancetype)clientWithDatabaseInfo:(const DatabaseInfo &)databaseInfo - usePersistence:(BOOL)usePersistence + settings:(FIRFirestoreSettings *)settings credentialsProvider: (CredentialsProvider *)credentialsProvider // no passing ownership userExecutor:(std::unique_ptr)userExecutor workerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue { return [[FSTFirestoreClient alloc] initWithDatabaseInfo:databaseInfo - usePersistence:usePersistence + settings:settings credentialsProvider:credentialsProvider userExecutor:std::move(userExecutor) workerDispatchQueue:workerDispatchQueue]; } - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo - usePersistence:(BOOL)usePersistence + settings:(FIRFirestoreSettings *)settings credentialsProvider: (CredentialsProvider *)credentialsProvider // no passing ownership userExecutor:(std::unique_ptr)userExecutor @@ -167,13 +168,13 @@ - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo // before any subsequently queued work runs. [_workerDispatchQueue dispatchAsync:^{ User user = userPromise->get_future().get(); - [self initializeWithUser:user usePersistence:usePersistence]; + [self initializeWithUser:user settings:settings]; }]; } return self; } -- (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistence { +- (void)initializeWithUser:(const User &)user settings:(FIRFirestoreSettings *)settings { // Do all of our initialization on our own dispatch queue. [self.workerDispatchQueue verifyIsCurrentQueue]; LOG_DEBUG("Initializing. Current user: %s", user.uid()); @@ -181,7 +182,7 @@ - (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistenc // Note: The initialization work must all be synchronous (we can't dispatch more work) since // external write/listen operations could get queued to run before that subsequent work // completes. - if (usePersistence) { + if (settings.isPersistenceEnabled) { Path dir = [FSTLevelDB storageDirectoryForDatabaseInfo:*self.databaseInfo documentsDirectory:[FSTLevelDB documentsDirectory]]; @@ -190,9 +191,10 @@ - (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistenc FSTLocalSerializer *serializer = [[FSTLocalSerializer alloc] initWithRemoteSerializer:remoteSerializer]; // TODO(gsoltis): enable LRU GC once API is finalized - FSTLevelDB *ldb = [[FSTLevelDB alloc] initWithDirectory:std::move(dir) - serializer:serializer - lruParams:LruParams::Disabled()]; + FSTLevelDB *ldb = + [[FSTLevelDB alloc] initWithDirectory:std::move(dir) + serializer:serializer + lruParams:LruParams::WithCacheSize(settings.cacheSizeBytes)]; _lruDelegate = ldb.referenceDelegate; _persistence = ldb; [self scheduleLruGarbageCollection]; diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index 3258cf13556..f1c52c233d3 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -44,6 +44,12 @@ struct LruParams { return LruParams{kFIRFirestoreCacheSizeUnlimited, 0, 0}; } + static LruParams WithCacheSize(int64_t cacheSize) { + LruParams params = Default(); + params.minBytesThreshold = cacheSize; + return params; + } + int64_t minBytesThreshold; int percentileToCollect; int maximumSequenceNumbersToCollect; diff --git a/Firestore/Source/Public/FIRFirestoreSettings.h b/Firestore/Source/Public/FIRFirestoreSettings.h index 919aadbab76..1e82134acdb 100644 --- a/Firestore/Source/Public/FIRFirestoreSettings.h +++ b/Firestore/Source/Public/FIRFirestoreSettings.h @@ -19,7 +19,7 @@ NS_ASSUME_NONNULL_BEGIN /** Used to set on-disk cache size to unlimited. Garbage collection will not run. */ -extern const int64_t kFIRFirestoreCacheSizeUnlimited; +extern const int64_t kFIRFirestoreCacheSizeUnlimited NS_SWIFT_NAME(FirestoreCacheSizeUnlimited); /** Settings used to configure a `FIRFirestore` instance. */ NS_SWIFT_NAME(FirestoreSettings) @@ -64,6 +64,15 @@ NS_SWIFT_NAME(FirestoreSettings) */ @property(nonatomic, getter=areTimestampsInSnapshotsEnabled) BOOL timestampsInSnapshotsEnabled; +/** + * Sets the cache size threshold above which the SDK will attempt to collect least-recently-used + * documents. The size is not a guarantee that the cache will stay below that size, only that if + * the cache exceeds the given size, cleanup will be attempted. Cannot be set lower than 1MB. + * + * Set to kFIRFirestoreCacheSizeUnlimited to disable garbage collection entirely. + */ +@property(nonatomic, assign) int64_t cacheSizeBytes; + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Swift/Tests/API/BasicCompileTests.swift b/Firestore/Swift/Tests/API/BasicCompileTests.swift index 16886e22955..2191de2cf2d 100644 --- a/Firestore/Swift/Tests/API/BasicCompileTests.swift +++ b/Firestore/Swift/Tests/API/BasicCompileTests.swift @@ -64,6 +64,7 @@ func initializeDb() -> Firestore { settings.host = "localhost" settings.isPersistenceEnabled = true settings.areTimestampsInSnapshotsEnabled = true + settings.cacheSizeBytes = FirestoreCacheSizeUnlimited firestore.settings = settings return firestore From 17fc9e28022eb94793b336a2dd132f1d9948c6e4 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Tue, 16 Oct 2018 15:18:49 -0700 Subject: [PATCH 24/33] Add second timer setup, use NSInterval, drop post-compaction --- Firestore/Source/Core/FSTFirestoreClient.mm | 13 ++++++------- Firestore/Source/Local/FSTLRUGarbageCollector.h | 10 ++-------- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 14 +++++--------- Firestore/Source/Local/FSTLevelDB.mm | 13 ++++++------- Firestore/Source/Local/FSTMemoryPersistence.mm | 12 ++++++------ Firestore/Source/Util/FSTDispatchQueue.mm | 1 + .../core/src/firebase/firestore/util/async_queue.h | 4 ++++ 7 files changed, 30 insertions(+), 37 deletions(-) diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 60b2b199e70..777904008ae 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -66,9 +66,9 @@ NS_ASSUME_NONNULL_BEGIN /** How long we wait to try running LRU GC after SDK initialization. */ -static const long FSTLruGcInitialDelay = 60 * 1000; +static const NSTimeInterval FSTLruGcInitialDelay = 60; /** Minimum amount of time between GC checks, after the first one. */ -static const long FSTLruGcRegularDelay = 5 * 60 * 1000; +static const NSTimeInterval FSTLruGcRegularDelay = 5 * 60; @interface FSTFirestoreClient () { DatabaseInfo _databaseInfo; @@ -103,8 +103,8 @@ - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo @implementation FSTFirestoreClient { std::unique_ptr _userExecutor; - long _initialGcDelay; - long _regularGcDelay; + NSTimeInterval _initialGcDelay; + NSTimeInterval _regularGcDelay; BOOL _gcHasRun; _Nullable id _lruDelegate; FSTDelayedCallback *_Nullable _lruCallback; @@ -189,10 +189,9 @@ - (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistenc [[FSTSerializerBeta alloc] initWithDatabaseID:&self.databaseInfo->database_id()]; FSTLocalSerializer *serializer = [[FSTLocalSerializer alloc] initWithRemoteSerializer:remoteSerializer]; - // TODO(gsoltis): enable LRU GC once API is finalized FSTLevelDB *ldb = [[FSTLevelDB alloc] initWithDirectory:std::move(dir) serializer:serializer - lruParams:LruParams::Disabled()]; + lruParams:LruParams::Default()]; _lruDelegate = ldb.referenceDelegate; _persistence = ldb; [self scheduleLruGarbageCollection]; @@ -242,7 +241,7 @@ - (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistenc * run. */ - (void)scheduleLruGarbageCollection { - long delay = _gcHasRun ? _initialGcDelay : _regularGcDelay; + NSTimeInterval delay = _gcHasRun ? _regularGcDelay : _initialGcDelay; _lruCallback = [_workerDispatchQueue dispatchAfterDelay:delay timerID:FSTTimerIDGarbageCollectionDelay diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index 3258cf13556..bea37d1b34c 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -101,14 +101,8 @@ struct LruResults { - (size_t)byteSize; -/** Returns the number of targets cached. */ -- (int32_t)targetCount; - -/** - * If available, persistence implementations should run any compaction that they can. This is done - * after GC has run to allow deletes to be processed and free up space. - */ -- (void)runPostCompaction; +/** Returns the number of targets and documents cached. */ +- (int32_t)sequenceNumberCount; /** Access to the underlying LRU Garbage collector instance. */ @property(strong, nonatomic, readonly) FSTLRUGarbageCollector *gc; diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 67f8a7aee0f..e1a35ac39ee 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -103,10 +103,11 @@ - (LruResults)collectWithLiveTargets:(NSDictionary * size_t currentSize = [self byteSize]; if (currentSize < _params.minBytesThreshold) { // Not enough on disk to warrant collection. Wait another timeout cycle. - LOG_DEBUG("Garbage collection skipped; Cache size %i is lower than threshold %i", currentSize, - _params.minBytesThreshold); + LOG_DEBUG("Garbage collection skipped; Cache size %s is lower than threshold %s", std::to_string(currentSize), + std::to_string(_params.minBytesThreshold)); return LruResults::DidNotRun(); } else { + LOG_DEBUG("Running garbage collection on cache of size: %s", std::to_string(currentSize)); return [self runGCWithLiveTargets:liveTargets]; } } @@ -130,9 +131,6 @@ - (LruResults)runGCWithLiveTargets:(NSDictionary *)l int numDocumentsRemoved = [self removeOrphanedDocumentsThroughSequenceNumber:upperBound]; Timestamp removedDocuments = Timestamp::Now(); - [_delegate runPostCompaction]; - Timestamp compactedDb = Timestamp::Now(); - std::string desc = "LRU Garbage Collection:\n"; absl::StrAppend(&desc, "\tCounted targets in ", millisecondsBetween(start, countedTargets), "ms\n"); @@ -143,16 +141,14 @@ - (LruResults)runGCWithLiveTargets:(NSDictionary *)l millisecondsBetween(foundUpperBound, removedTargets), "ms\n"); absl::StrAppend(&desc, "\tRemoved ", numDocumentsRemoved, " documents in ", millisecondsBetween(removedTargets, removedDocuments), "ms\n"); - absl::StrAppend(&desc, "\tCompacted leveldb database in ", - millisecondsBetween(removedDocuments, compactedDb), "ms\n"); - absl::StrAppend(&desc, "Total duration: ", millisecondsBetween(start, compactedDb), "ms"); + absl::StrAppend(&desc, "Total duration: ", millisecondsBetween(start, removedDocuments), "ms"); LOG_DEBUG(desc.c_str()); return LruResults{/* didRun= */ true, sequenceNumbers, numTargetsRemoved, numDocumentsRemoved}; } - (int)queryCountForPercentile:(NSUInteger)percentile { - int totalCount = [_delegate targetCount]; + int totalCount = [_delegate sequenceNumberCount]; int setSize = (int)((percentile / 100.0f) * totalCount); return setSize; } diff --git a/Firestore/Source/Local/FSTLevelDB.mm b/Firestore/Source/Local/FSTLevelDB.mm index e8f8388b467..b195deed325 100644 --- a/Firestore/Source/Local/FSTLevelDB.mm +++ b/Firestore/Source/Local/FSTLevelDB.mm @@ -229,13 +229,12 @@ - (int)removeTargetsThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber return [queryCache removeQueriesThroughSequenceNumber:sequenceNumber liveQueries:liveQueries]; } -- (int32_t)targetCount { - return [_db.queryCache count]; -} - -- (void)runPostCompaction { - // Compacts the entire db - _db.ptr->CompactRange(NULL, NULL); +- (int32_t)sequenceNumberCount { + __block int32_t totalCount = [_db.queryCache count]; + [self enumerateMutationsUsingBlock:^(const DocumentKey &key, ListenSequenceNumber sequenceNumber, BOOL *stop) { + totalCount++; + }]; + return totalCount; } - (FSTLRUGarbageCollector *)gc { diff --git a/Firestore/Source/Local/FSTMemoryPersistence.mm b/Firestore/Source/Local/FSTMemoryPersistence.mm index ac9606bdd48..97eadde82ee 100644 --- a/Firestore/Source/Local/FSTMemoryPersistence.mm +++ b/Firestore/Source/Local/FSTMemoryPersistence.mm @@ -244,12 +244,12 @@ - (int)removeTargetsThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber liveQueries:liveQueries]; } -- (int32_t)targetCount { - return [_persistence.queryCache count]; -} - -- (void)runPostCompaction { - // No-op for memory persistence. +- (int32_t)sequenceNumberCount { + __block int32_t totalCount = [_persistence.queryCache count]; + [self enumerateMutationsUsingBlock:^(const DocumentKey &key, ListenSequenceNumber sequenceNumber, BOOL *stop) { + totalCount++; + }]; + return totalCount; } - (int)removeOrphanedDocumentsThroughSequenceNumber:(ListenSequenceNumber)upperBound { diff --git a/Firestore/Source/Util/FSTDispatchQueue.mm b/Firestore/Source/Util/FSTDispatchQueue.mm index b921484f84a..24e6f9b3996 100644 --- a/Firestore/Source/Util/FSTDispatchQueue.mm +++ b/Firestore/Source/Util/FSTDispatchQueue.mm @@ -72,6 +72,7 @@ + (TimerId)convertTimerId:(FSTTimerID)objcTimerID { case TimerId::WriteStreamIdle: case TimerId::WriteStreamConnectionBackoff: case TimerId::OnlineStateTimeout: + case TimerId::GarbageCollectionDelay: return converted; default: HARD_FAIL("Unknown value of enum FSTTimerID."); diff --git a/Firestore/core/src/firebase/firestore/util/async_queue.h b/Firestore/core/src/firebase/firestore/util/async_queue.h index 8f3e1ad8303..3360f9969da 100644 --- a/Firestore/core/src/firebase/firestore/util/async_queue.h +++ b/Firestore/core/src/firebase/firestore/util/async_queue.h @@ -54,6 +54,10 @@ enum class TimerId { * indefinitely for success or failure. */ OnlineStateTimeout, + /** + * A timer used to periodically attempt LRU Garbage collection + */ + GarbageCollectionDelay }; // A serial queue that executes given operations asynchronously, one at a time. From 47cc5b43b8f731be5ee698b186df57ff202c897d Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Tue, 16 Oct 2018 15:39:11 -0700 Subject: [PATCH 25/33] Update comment re orphaned documents --- Firestore/Source/Local/FSTLRUGarbageCollector.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index cf5eaa60501..1eeb5e6bd32 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -107,7 +107,7 @@ struct LruResults { - (size_t)byteSize; -/** Returns the number of targets and documents cached. */ +/** Returns the number of targets and orphaned documents cached. */ - (int32_t)sequenceNumberCount; /** Access to the underlying LRU Garbage collector instance. */ From f20c50a9f3c76258ed75ba90502f4511c2e1f994 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Tue, 16 Oct 2018 15:41:40 -0700 Subject: [PATCH 26/33] Style --- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 4 ++-- Firestore/Source/Local/FSTLevelDB.mm | 3 ++- Firestore/Source/Local/FSTMemoryPersistence.mm | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index e1a35ac39ee..44fcdccf85b 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -103,8 +103,8 @@ - (LruResults)collectWithLiveTargets:(NSDictionary * size_t currentSize = [self byteSize]; if (currentSize < _params.minBytesThreshold) { // Not enough on disk to warrant collection. Wait another timeout cycle. - LOG_DEBUG("Garbage collection skipped; Cache size %s is lower than threshold %s", std::to_string(currentSize), - std::to_string(_params.minBytesThreshold)); + LOG_DEBUG("Garbage collection skipped; Cache size %s is lower than threshold %s", + std::to_string(currentSize), std::to_string(_params.minBytesThreshold)); return LruResults::DidNotRun(); } else { LOG_DEBUG("Running garbage collection on cache of size: %s", std::to_string(currentSize)); diff --git a/Firestore/Source/Local/FSTLevelDB.mm b/Firestore/Source/Local/FSTLevelDB.mm index b195deed325..3087d244b49 100644 --- a/Firestore/Source/Local/FSTLevelDB.mm +++ b/Firestore/Source/Local/FSTLevelDB.mm @@ -231,7 +231,8 @@ - (int)removeTargetsThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber - (int32_t)sequenceNumberCount { __block int32_t totalCount = [_db.queryCache count]; - [self enumerateMutationsUsingBlock:^(const DocumentKey &key, ListenSequenceNumber sequenceNumber, BOOL *stop) { + [self enumerateMutationsUsingBlock:^(const DocumentKey &key, ListenSequenceNumber sequenceNumber, + BOOL *stop) { totalCount++; }]; return totalCount; diff --git a/Firestore/Source/Local/FSTMemoryPersistence.mm b/Firestore/Source/Local/FSTMemoryPersistence.mm index 97eadde82ee..e8f729a0827 100644 --- a/Firestore/Source/Local/FSTMemoryPersistence.mm +++ b/Firestore/Source/Local/FSTMemoryPersistence.mm @@ -246,7 +246,8 @@ - (int)removeTargetsThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber - (int32_t)sequenceNumberCount { __block int32_t totalCount = [_persistence.queryCache count]; - [self enumerateMutationsUsingBlock:^(const DocumentKey &key, ListenSequenceNumber sequenceNumber, BOOL *stop) { + [self enumerateMutationsUsingBlock:^(const DocumentKey &key, ListenSequenceNumber sequenceNumber, + BOOL *stop) { totalCount++; }]; return totalCount; From 3a1bc209233b088b28f16ab5d61ffa808daf3866 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Wed, 17 Oct 2018 15:59:48 -0700 Subject: [PATCH 27/33] Remove unnecessary to_string calls --- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 44fcdccf85b..31919a375d0 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -103,11 +103,11 @@ - (LruResults)collectWithLiveTargets:(NSDictionary * size_t currentSize = [self byteSize]; if (currentSize < _params.minBytesThreshold) { // Not enough on disk to warrant collection. Wait another timeout cycle. - LOG_DEBUG("Garbage collection skipped; Cache size %s is lower than threshold %s", - std::to_string(currentSize), std::to_string(_params.minBytesThreshold)); + LOG_DEBUG("Garbage collection skipped; Cache size %s is lower than threshold %s", currentSize, + _params.minBytesThreshold); return LruResults::DidNotRun(); } else { - LOG_DEBUG("Running garbage collection on cache of size: %s", std::to_string(currentSize)); + LOG_DEBUG("Running garbage collection on cache of size: %s", currentSize); return [self runGCWithLiveTargets:liveTargets]; } } From ba406b652636be7f3841646410174b40c997a895 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Wed, 17 Oct 2018 16:09:36 -0700 Subject: [PATCH 28/33] LRU tweaks (#1961) * Add second timer setup, use NSInterval, drop post-compaction --- Firestore/Source/Core/FSTFirestoreClient.mm | 11 +++++------ Firestore/Source/Local/FSTLRUGarbageCollector.h | 10 ++-------- Firestore/Source/Local/FSTLRUGarbageCollector.mm | 12 ++++-------- Firestore/Source/Local/FSTLevelDB.mm | 14 +++++++------- Firestore/Source/Local/FSTMemoryPersistence.mm | 13 +++++++------ Firestore/Source/Util/FSTDispatchQueue.mm | 1 + .../core/src/firebase/firestore/util/async_queue.h | 4 ++++ 7 files changed, 30 insertions(+), 35 deletions(-) diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 7cd6c9879e7..2cc201585b4 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -67,9 +67,9 @@ NS_ASSUME_NONNULL_BEGIN /** How long we wait to try running LRU GC after SDK initialization. */ -static const long FSTLruGcInitialDelay = 60 * 1000; +static const NSTimeInterval FSTLruGcInitialDelay = 60; /** Minimum amount of time between GC checks, after the first one. */ -static const long FSTLruGcRegularDelay = 5 * 60 * 1000; +static const NSTimeInterval FSTLruGcRegularDelay = 5 * 60; @interface FSTFirestoreClient () { DatabaseInfo _databaseInfo; @@ -104,8 +104,8 @@ - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo @implementation FSTFirestoreClient { std::unique_ptr _userExecutor; - long _initialGcDelay; - long _regularGcDelay; + NSTimeInterval _initialGcDelay; + NSTimeInterval _regularGcDelay; BOOL _gcHasRun; _Nullable id _lruDelegate; FSTDelayedCallback *_Nullable _lruCallback; @@ -190,7 +190,6 @@ - (void)initializeWithUser:(const User &)user settings:(FIRFirestoreSettings *)s [[FSTSerializerBeta alloc] initWithDatabaseID:&self.databaseInfo->database_id()]; FSTLocalSerializer *serializer = [[FSTLocalSerializer alloc] initWithRemoteSerializer:remoteSerializer]; - // TODO(gsoltis): enable LRU GC once API is finalized FSTLevelDB *ldb = [[FSTLevelDB alloc] initWithDirectory:std::move(dir) serializer:serializer @@ -244,7 +243,7 @@ - (void)initializeWithUser:(const User &)user settings:(FIRFirestoreSettings *)s * run. */ - (void)scheduleLruGarbageCollection { - long delay = _gcHasRun ? _initialGcDelay : _regularGcDelay; + NSTimeInterval delay = _gcHasRun ? _regularGcDelay : _initialGcDelay; _lruCallback = [_workerDispatchQueue dispatchAfterDelay:delay timerID:FSTTimerIDGarbageCollectionDelay diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.h b/Firestore/Source/Local/FSTLRUGarbageCollector.h index f1c52c233d3..1eeb5e6bd32 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.h +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.h @@ -107,14 +107,8 @@ struct LruResults { - (size_t)byteSize; -/** Returns the number of targets cached. */ -- (int32_t)targetCount; - -/** - * If available, persistence implementations should run any compaction that they can. This is done - * after GC has run to allow deletes to be processed and free up space. - */ -- (void)runPostCompaction; +/** Returns the number of targets and orphaned documents cached. */ +- (int32_t)sequenceNumberCount; /** Access to the underlying LRU Garbage collector instance. */ @property(strong, nonatomic, readonly) FSTLRUGarbageCollector *gc; diff --git a/Firestore/Source/Local/FSTLRUGarbageCollector.mm b/Firestore/Source/Local/FSTLRUGarbageCollector.mm index 67f8a7aee0f..31919a375d0 100644 --- a/Firestore/Source/Local/FSTLRUGarbageCollector.mm +++ b/Firestore/Source/Local/FSTLRUGarbageCollector.mm @@ -103,10 +103,11 @@ - (LruResults)collectWithLiveTargets:(NSDictionary * size_t currentSize = [self byteSize]; if (currentSize < _params.minBytesThreshold) { // Not enough on disk to warrant collection. Wait another timeout cycle. - LOG_DEBUG("Garbage collection skipped; Cache size %i is lower than threshold %i", currentSize, + LOG_DEBUG("Garbage collection skipped; Cache size %s is lower than threshold %s", currentSize, _params.minBytesThreshold); return LruResults::DidNotRun(); } else { + LOG_DEBUG("Running garbage collection on cache of size: %s", currentSize); return [self runGCWithLiveTargets:liveTargets]; } } @@ -130,9 +131,6 @@ - (LruResults)runGCWithLiveTargets:(NSDictionary *)l int numDocumentsRemoved = [self removeOrphanedDocumentsThroughSequenceNumber:upperBound]; Timestamp removedDocuments = Timestamp::Now(); - [_delegate runPostCompaction]; - Timestamp compactedDb = Timestamp::Now(); - std::string desc = "LRU Garbage Collection:\n"; absl::StrAppend(&desc, "\tCounted targets in ", millisecondsBetween(start, countedTargets), "ms\n"); @@ -143,16 +141,14 @@ - (LruResults)runGCWithLiveTargets:(NSDictionary *)l millisecondsBetween(foundUpperBound, removedTargets), "ms\n"); absl::StrAppend(&desc, "\tRemoved ", numDocumentsRemoved, " documents in ", millisecondsBetween(removedTargets, removedDocuments), "ms\n"); - absl::StrAppend(&desc, "\tCompacted leveldb database in ", - millisecondsBetween(removedDocuments, compactedDb), "ms\n"); - absl::StrAppend(&desc, "Total duration: ", millisecondsBetween(start, compactedDb), "ms"); + absl::StrAppend(&desc, "Total duration: ", millisecondsBetween(start, removedDocuments), "ms"); LOG_DEBUG(desc.c_str()); return LruResults{/* didRun= */ true, sequenceNumbers, numTargetsRemoved, numDocumentsRemoved}; } - (int)queryCountForPercentile:(NSUInteger)percentile { - int totalCount = [_delegate targetCount]; + int totalCount = [_delegate sequenceNumberCount]; int setSize = (int)((percentile / 100.0f) * totalCount); return setSize; } diff --git a/Firestore/Source/Local/FSTLevelDB.mm b/Firestore/Source/Local/FSTLevelDB.mm index e8f8388b467..3087d244b49 100644 --- a/Firestore/Source/Local/FSTLevelDB.mm +++ b/Firestore/Source/Local/FSTLevelDB.mm @@ -229,13 +229,13 @@ - (int)removeTargetsThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber return [queryCache removeQueriesThroughSequenceNumber:sequenceNumber liveQueries:liveQueries]; } -- (int32_t)targetCount { - return [_db.queryCache count]; -} - -- (void)runPostCompaction { - // Compacts the entire db - _db.ptr->CompactRange(NULL, NULL); +- (int32_t)sequenceNumberCount { + __block int32_t totalCount = [_db.queryCache count]; + [self enumerateMutationsUsingBlock:^(const DocumentKey &key, ListenSequenceNumber sequenceNumber, + BOOL *stop) { + totalCount++; + }]; + return totalCount; } - (FSTLRUGarbageCollector *)gc { diff --git a/Firestore/Source/Local/FSTMemoryPersistence.mm b/Firestore/Source/Local/FSTMemoryPersistence.mm index ac9606bdd48..e8f729a0827 100644 --- a/Firestore/Source/Local/FSTMemoryPersistence.mm +++ b/Firestore/Source/Local/FSTMemoryPersistence.mm @@ -244,12 +244,13 @@ - (int)removeTargetsThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber liveQueries:liveQueries]; } -- (int32_t)targetCount { - return [_persistence.queryCache count]; -} - -- (void)runPostCompaction { - // No-op for memory persistence. +- (int32_t)sequenceNumberCount { + __block int32_t totalCount = [_persistence.queryCache count]; + [self enumerateMutationsUsingBlock:^(const DocumentKey &key, ListenSequenceNumber sequenceNumber, + BOOL *stop) { + totalCount++; + }]; + return totalCount; } - (int)removeOrphanedDocumentsThroughSequenceNumber:(ListenSequenceNumber)upperBound { diff --git a/Firestore/Source/Util/FSTDispatchQueue.mm b/Firestore/Source/Util/FSTDispatchQueue.mm index b921484f84a..24e6f9b3996 100644 --- a/Firestore/Source/Util/FSTDispatchQueue.mm +++ b/Firestore/Source/Util/FSTDispatchQueue.mm @@ -72,6 +72,7 @@ + (TimerId)convertTimerId:(FSTTimerID)objcTimerID { case TimerId::WriteStreamIdle: case TimerId::WriteStreamConnectionBackoff: case TimerId::OnlineStateTimeout: + case TimerId::GarbageCollectionDelay: return converted; default: HARD_FAIL("Unknown value of enum FSTTimerID."); diff --git a/Firestore/core/src/firebase/firestore/util/async_queue.h b/Firestore/core/src/firebase/firestore/util/async_queue.h index 8f3e1ad8303..3360f9969da 100644 --- a/Firestore/core/src/firebase/firestore/util/async_queue.h +++ b/Firestore/core/src/firebase/firestore/util/async_queue.h @@ -54,6 +54,10 @@ enum class TimerId { * indefinitely for success or failure. */ OnlineStateTimeout, + /** + * A timer used to periodically attempt LRU Garbage collection + */ + GarbageCollectionDelay }; // A serial queue that executes given operations asynchronously, one at a time. From 0bdfb984669b7b04a39ff0f53ed3530d2c97807f Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Sun, 25 Nov 2018 20:41:30 -0800 Subject: [PATCH 29/33] Style --- Firestore/Source/Core/FSTFirestoreClient.mm | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index ee28bce09f5..91ce8827b0d 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -251,11 +251,12 @@ - (void)initializeWithUser:(const User &)user settings:(FIRFirestoreSettings *)s */ - (void)scheduleLruGarbageCollection { std::chrono::milliseconds delay = _gcHasRun ? _regularGcDelay : _initialGcDelay; - _lruCallback = absl::make_unique(_workerQueue->EnqueueAfterDelay(delay, TimerId::GarbageCollectionDelay, [&]() { - [self->_localStore collectGarbage:self->_lruDelegate.gc]; - self->_gcHasRun = YES; - [self scheduleLruGarbageCollection]; - })); + _lruCallback = absl::make_unique( + _workerQueue->EnqueueAfterDelay(delay, TimerId::GarbageCollectionDelay, [&]() { + [self->_localStore collectGarbage:self->_lruDelegate.gc]; + self->_gcHasRun = YES; + [self scheduleLruGarbageCollection]; + })); } - (void)credentialDidChangeWithUser:(const User &)user { From 3107b3a85d52502373d5a64418e15d02bc39c978 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Sun, 25 Nov 2018 20:51:45 -0800 Subject: [PATCH 30/33] pod update --- Firestore/Example/Firestore.xcodeproj/project.pbxproj | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj index b228b8707a1..edbdbad2c49 100644 --- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj +++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj @@ -1526,11 +1526,11 @@ ); inputPaths = ( "${SRCROOT}/Pods/Target Support Files/Pods-Firestore_Example_iOS/Pods-Firestore_Example_iOS-resources.sh", - "${PODS_CONFIGURATION_BUILD_DIR}/FirebaseFirestore/gRPCCertificates.bundle", + "${PODS_CONFIGURATION_BUILD_DIR}/FirebaseFirestore/gRPCCertificates-Firestore.bundle", ); name = "[CP] Copy Pods Resources"; outputPaths = ( - "${TARGET_BUILD_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/gRPCCertificates.bundle", + "${TARGET_BUILD_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/gRPCCertificates-Firestore.bundle", ); runOnlyForDeploymentPostprocessing = 0; shellPath = /bin/sh; @@ -1785,11 +1785,11 @@ ); inputPaths = ( "${SRCROOT}/Pods/Target Support Files/Pods-Firestore_Example_iOS-Firestore_SwiftTests_iOS/Pods-Firestore_Example_iOS-Firestore_SwiftTests_iOS-resources.sh", - "${PODS_CONFIGURATION_BUILD_DIR}/FirebaseFirestore/gRPCCertificates.bundle", + "${PODS_CONFIGURATION_BUILD_DIR}/FirebaseFirestore/gRPCCertificates-Firestore.bundle", ); name = "[CP] Copy Pods Resources"; outputPaths = ( - "${TARGET_BUILD_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/gRPCCertificates.bundle", + "${TARGET_BUILD_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/gRPCCertificates-Firestore.bundle", ); runOnlyForDeploymentPostprocessing = 0; shellPath = /bin/sh; From d3554375d145b7c09a90a2d7318ac9c2631b5f9b Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Sun, 25 Nov 2018 20:56:34 -0800 Subject: [PATCH 31/33] Fix lint tag --- Firestore/Source/Core/FSTFirestoreClient.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 91ce8827b0d..4671faae8d4 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -16,7 +16,7 @@ #import "Firestore/Source/Core/FSTFirestoreClient.h" -#include // NOLINY(build/c++11) +#include // NOLINT(build/c++11) #include // NOLINT(build/c++11) #include #include From 40c34aa4eff0b46a51ce64ef732415fa254c6d05 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Sun, 25 Nov 2018 21:32:55 -0800 Subject: [PATCH 32/33] Fix merge conflict --- Firestore/Source/Core/FSTFirestoreClient.mm | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 5bbd1d8b78f..4671faae8d4 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -108,13 +108,8 @@ @implementation FSTFirestoreClient { std::unique_ptr _workerQueue; std::unique_ptr _userExecutor; -<<<<<<< HEAD std::chrono::milliseconds _initialGcDelay; std::chrono::milliseconds _regularGcDelay; -======= - NSTimeInterval _initialGcDelay; - NSTimeInterval _regularGcDelay; ->>>>>>> lru_gc_disabled BOOL _gcHasRun; _Nullable id _lruDelegate; std::unique_ptr _lruCallback; From e48527715ed365c099d6b311f413da5ec14808ef Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Mon, 26 Nov 2018 13:17:23 -0800 Subject: [PATCH 33/33] Use DelayedOperation directly, since it can be internally empty --- Firestore/Source/Core/FSTFirestoreClient.mm | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 4671faae8d4..c72eae6cc18 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -112,7 +112,7 @@ @implementation FSTFirestoreClient { std::chrono::milliseconds _regularGcDelay; BOOL _gcHasRun; _Nullable id _lruDelegate; - std::unique_ptr _lruCallback; + DelayedOperation _lruCallback; } - (Executor *)userExecutor { @@ -251,12 +251,11 @@ - (void)initializeWithUser:(const User &)user settings:(FIRFirestoreSettings *)s */ - (void)scheduleLruGarbageCollection { std::chrono::milliseconds delay = _gcHasRun ? _regularGcDelay : _initialGcDelay; - _lruCallback = absl::make_unique( - _workerQueue->EnqueueAfterDelay(delay, TimerId::GarbageCollectionDelay, [&]() { - [self->_localStore collectGarbage:self->_lruDelegate.gc]; - self->_gcHasRun = YES; - [self scheduleLruGarbageCollection]; - })); + _lruCallback = _workerQueue->EnqueueAfterDelay(delay, TimerId::GarbageCollectionDelay, [self]() { + [self->_localStore collectGarbage:self->_lruDelegate.gc]; + self->_gcHasRun = YES; + [self scheduleLruGarbageCollection]; + }); } - (void)credentialDidChangeWithUser:(const User &)user { @@ -294,8 +293,7 @@ - (void)shutdownWithCompletion:(nullable FSTVoidErrorBlock)completion { // If we've scheduled LRU garbage collection, cancel it. if (self->_lruCallback) { - self->_lruCallback->Cancel(); - self->_lruCallback.reset(); + self->_lruCallback.Cancel(); } [self.remoteStore shutdown]; [self.persistence shutdown];