From 17fc9e28022eb94793b336a2dd132f1d9948c6e4 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Tue, 16 Oct 2018 15:18:49 -0700 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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]; } }