Skip to content

Wire together LRU GC #1905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 41 commits into from
Nov 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
ba07d15
Wire together LRU GC
Oct 3, 2018
46b6a95
Lint
Oct 3, 2018
ab90dd6
Fix declarations
Oct 3, 2018
b4ec0dc
Renaming, and using C++ namespaces
Oct 4, 2018
95e6f77
add NS tags to initializers, switch gc call to collect
Oct 5, 2018
42e1566
Remove timing from lru results struct
Oct 5, 2018
c2ace26
Switch to chrono for times
Oct 5, 2018
efac4fd
Move LruResults in C++ namespaces
Oct 5, 2018
eaa682f
Rename localstore garbage collection method
Oct 5, 2018
ee2cd93
Style
Oct 5, 2018
6b707ba
exempt chrono from linting
Oct 5, 2018
f8c5df3
Merge in master
Oct 5, 2018
dc948b9
Style
Oct 5, 2018
9aec574
BOOL -> bool
Oct 5, 2018
be136d3
Define unlimited cache size for ObjC
Oct 5, 2018
618f329
Fix type
Oct 5, 2018
161df33
Drop unused fields
Oct 8, 2018
e5259ad
Technically calls to GC should be in a transaction, the tests passed …
Oct 8, 2018
e0cda8c
Add transaction in local store as well
Oct 8, 2018
8622c2b
Fix subtle unsigned bug in test
Oct 8, 2018
6d13c58
Switch to serializedSize method
Oct 8, 2018
55a7608
Fix in-memory lru orphan calculation
Oct 8, 2018
c3098d6
Style
Oct 10, 2018
9894353
Wire up LRU GC (#1925)
Oct 11, 2018
bdee384
Merge branch 'master' into lru_with_master
Oct 15, 2018
17fc9e2
Add second timer setup, use NSInterval, drop post-compaction
Oct 16, 2018
2cc5374
Merge branch 'master' into lru_gc_disabled
Oct 16, 2018
ba76b15
Merge branch 'gsoltis/lru_gc_disabled' of https://github.com/firebase…
Oct 16, 2018
70f945b
Merge in upstream
Oct 16, 2018
47cc5b4
Update comment re orphaned documents
Oct 16, 2018
f20c50a
Style
Oct 16, 2018
3a1bc20
Remove unnecessary to_string calls
Oct 17, 2018
ba406b6
LRU tweaks (#1961)
Oct 17, 2018
515b0c3
Merging in master
Nov 26, 2018
0bdfb98
Style
Nov 26, 2018
6297c33
Merge branch 'master' into lru_with_master
Nov 26, 2018
3107b3a
pod update
Nov 26, 2018
d355437
Fix lint tag
Nov 26, 2018
7a70f23
Merge in upstream
Nov 26, 2018
40c34aa
Fix merge conflict
Nov 26, 2018
e485277
Use DelayedOperation directly, since it can be internally empty
Nov 26, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Firestore/Example/Firestore.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@

#import <XCTest/XCTest.h>

#import "Firestore/Source/Local/FSTLRUGarbageCollector.h"

@protocol FSTPersistence;

NS_ASSUME_NONNULL_BEGIN

@interface FSTLRUGarbageCollectorTests : XCTestCase

- (id<FSTPersistence>)newPersistence;
- (id<FSTPersistence>)newPersistenceWithLruParams:(firebase::firestore::local::LruParams)lruParams;

@property(nonatomic, strong) id<FSTPersistence> persistence;

Expand Down
93 changes: 90 additions & 3 deletions Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@

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;
Expand Down Expand Up @@ -79,9 +81,9 @@ - (BOOL)isTestBaseClass {
return ([self class] == [FSTLRUGarbageCollectorTests class]);
}

- (void)newTestResources {
- (void)newTestResourcesWithLruParams:(LruParams)lruParams {
HARD_ASSERT(_persistence == nil, "Persistence already created");
_persistence = [self newPersistence];
_persistence = [self newPersistenceWithLruParams:lruParams];
_queryCache = [_persistence queryCache];
_documentCache = [_persistence remoteDocumentCache];
_mutationQueue = [_persistence mutationQueueForUser:_user];
Expand All @@ -93,7 +95,11 @@ - (void)newTestResources {
});
}

- (id<FSTPersistence>)newPersistence {
- (void)newTestResources {
[self newTestResourcesWithLruParams:LruParams::Default()];
}

- (id<FSTPersistence>)newPersistenceWithLruParams:(LruParams)lruParams {
@throw FSTAbstractMethodException(); // NOLINT
}

Expand Down Expand Up @@ -630,6 +636,7 @@ - (void)testRemoveTargetsThenGC {
// Finally, do the garbage collection, up to but not including the removal of middleTarget
NSDictionary<NSNumber *, FSTQueryData *> *liveQueries =
@{@(oldestTarget.targetID) : oldestTarget};

int queriesRemoved = [self removeQueriesThroughSequenceNumber:upperBound liveQueries:liveQueries];
XCTAssertEqual(1, queriesRemoved, @"Expected to remove newest target");
int docsRemoved = [self removeOrphanedDocumentsThroughSequenceNumber:upperBound];
Expand Down Expand Up @@ -672,6 +679,86 @@ - (void)testGetsSize {
[_persistence shutdown];
}

- (void)testDisabled {
if ([self isTestBaseClass]) return;

LruParams params = LruParams::Disabled();
[self newTestResourcesWithLruParams: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];
}
});

LruResults results =
_persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; });
XCTAssertFalse(results.didRun);

[_persistence shutdown];
}

- (void)testCacheTooSmall {
if ([self isTestBaseClass]) return;

LruParams params = LruParams::Default();
[self newTestResourcesWithLruParams: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];
}
});

int cacheSize = (int)[_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
LruResults results =
_persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; });
XCTAssertFalse(results.didRun);

[_persistence shutdown];
}

- (void)testGCRan {
if ([self isTestBaseClass]) return;

LruParams params = LruParams::Default();
// Set a low threshold so we will definitely run
params.minBytesThreshold = 100;
[self newTestResourcesWithLruParams: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.
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.
XCTAssertTrue(results.didRun);
XCTAssertEqual(10, results.targetsRemoved);
XCTAssertEqual(100, results.documentsRemoved);
[_persistence shutdown];
}

@end

NS_ASSUME_NONNULL_END
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,25 @@
#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"
#include "Firestore/core/src/firebase/firestore/local/leveldb_key.h"
#include "Firestore/core/src/firebase/firestore/model/document_key.h"

using firebase::firestore::local::LevelDbDocumentTargetKey;
using firebase::firestore::model::DocumentKey;

using firebase::firestore::local::LruParams;

NS_ASSUME_NONNULL_BEGIN

@interface FSTLevelDBLRUGarbageCollectorTests : FSTLRUGarbageCollectorTests
@end

@implementation FSTLevelDBLRUGarbageCollectorTests

- (id<FSTPersistence>)newPersistence {
return [FSTPersistenceTestHelpers levelDBPersistence];
- (id<FSTPersistence>)newPersistenceWithLruParams:(LruParams)lruParams {
return [FSTPersistenceTestHelpers levelDBPersistenceWithLruParams:lruParams];
}

- (BOOL)sentinelExists:(const DocumentKey &)key {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,23 @@
#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"
#include "Firestore/core/src/firebase/firestore/model/document_key.h"

using firebase::firestore::model::DocumentKey;

using firebase::firestore::local::LruParams;

NS_ASSUME_NONNULL_BEGIN

@interface FSTMemoryLRUGarbageCollectionTests : FSTLRUGarbageCollectorTests
@end

@implementation FSTMemoryLRUGarbageCollectionTests

- (id<FSTPersistence>)newPersistence {
return [FSTPersistenceTestHelpers lruMemoryPersistence];
- (id<FSTPersistence>)newPersistenceWithLruParams:(LruParams)lruParams {
return [FSTPersistenceTestHelpers lruMemoryPersistenceWithLruParams:lruParams];
}

- (BOOL)sentinelExists:(const DocumentKey &)key {
Expand Down
12 changes: 12 additions & 0 deletions Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#import <Foundation/Foundation.h>

#import "Firestore/Source/Local/FSTLRUGarbageCollector.h"
#include "Firestore/core/src/firebase/firestore/util/path.h"

@class FSTLevelDB;
Expand Down Expand Up @@ -48,10 +49,21 @@ 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 *)levelDBPersistenceWithLruParams:(firebase::firestore::local::LruParams)lruParams;

/** Creates and starts a new FSTMemoryPersistence instance for testing. */
+ (FSTMemoryPersistence *)eagerGCMemoryPersistence;

+ (FSTMemoryPersistence *)lruMemoryPersistence;

+ (FSTMemoryPersistence *)lruMemoryPersistenceWithLruParams:
(firebase::firestore::local::LruParams)lruParams;
@end

NS_ASSUME_NONNULL_END
19 changes: 17 additions & 2 deletions Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include <utility>

#import "Firestore/Source/Local/FSTLRUGarbageCollector.h"
#import "Firestore/Source/Local/FSTLevelDB.h"
#import "Firestore/Source/Local/FSTLocalSerializer.h"
#import "Firestore/Source/Local/FSTMemoryPersistence.h"
Expand All @@ -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;
Expand Down Expand Up @@ -61,8 +63,13 @@ + (Path)levelDBDir {
}

+ (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir {
return [self levelDBPersistenceWithDir:dir lruParams:LruParams::Default()];
}

+ (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir lruParams:(LruParams)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 lruParams:params];
Status status = [db start];
if (!status.ok()) {
[NSException raise:NSInternalInconsistencyException
Expand All @@ -72,6 +79,10 @@ + (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir {
return db;
}

+ (FSTLevelDB *)levelDBPersistenceWithLruParams:(LruParams)lruParams {
return [self levelDBPersistenceWithDir:[self levelDBDir] lruParams:lruParams];
}

+ (FSTLevelDB *)levelDBPersistence {
return [self levelDBPersistenceWithDir:[self levelDBDir]];
}
Expand All @@ -88,9 +99,13 @@ + (FSTMemoryPersistence *)eagerGCMemoryPersistence {
}

+ (FSTMemoryPersistence *)lruMemoryPersistence {
return [self lruMemoryPersistenceWithLruParams:LruParams::Default()];
}

+ (FSTMemoryPersistence *)lruMemoryPersistenceWithLruParams:(LruParams)lruParams {
FSTLocalSerializer *serializer = [self localSerializer];
FSTMemoryPersistence *persistence =
[FSTMemoryPersistence persistenceWithLRUGCAndSerializer:serializer];
[FSTMemoryPersistence persistenceWithLruParams:lruParams serializer:serializer];
Status status = [persistence start];
if (!status.ok()) {
[NSException raise:NSInternalInconsistencyException
Expand Down
2 changes: 1 addition & 1 deletion Firestore/Source/API/FIRFirestore.mm
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ const DatabaseInfo database_info(*self.databaseID, util::MakeString(_persistence

HARD_ASSERT(_workerQueue, "Expected non-null _workerQueue");
_client = [FSTFirestoreClient clientWithDatabaseInfo:database_info
usePersistence:_settings.persistenceEnabled
settings:_settings
credentialsProvider:_credentialsProvider.get()
userExecutor:std::move(userExecutor)
workerQueue:std::move(_workerQueue)];
Expand Down
16 changes: 15 additions & 1 deletion Firestore/Source/API/FIRFirestoreSettings.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -35,6 +37,7 @@ - (instancetype)init {
_dispatchQueue = dispatch_get_main_queue();
_persistenceEnabled = kDefaultPersistenceEnabled;
_timestampsInSnapshotsEnabled = kDefaultTimestampsInSnapshotsEnabled;
_cacheSizeBytes = kDefaultCacheSizeBytes;
}
return self;
}
Expand All @@ -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 {
Expand All @@ -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;
}

Expand All @@ -70,6 +75,7 @@ - (id)copyWithZone:(nullable NSZone *)zone {
copy.dispatchQueue = _dispatchQueue;
copy.persistenceEnabled = _persistenceEnabled;
copy.timestampsInSnapshotsEnabled = _timestampsInSnapshotsEnabled;
copy.cacheSizeBytes = _cacheSizeBytes;
return copy;
}

Expand All @@ -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
3 changes: 2 additions & 1 deletion Firestore/Source/Core/FSTFirestoreClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

@class FIRDocumentReference;
@class FIRDocumentSnapshot;
@class FIRFirestoreSettings;
@class FIRQuery;
@class FIRQuerySnapshot;
@class FSTDatabaseID;
Expand Down Expand Up @@ -57,7 +58,7 @@ NS_ASSUME_NONNULL_BEGIN
*/
+ (instancetype)
clientWithDatabaseInfo:(const firebase::firestore::core::DatabaseInfo &)databaseInfo
usePersistence:(BOOL)usePersistence
settings:(FIRFirestoreSettings *)settings
credentialsProvider:(firebase::firestore::auth::CredentialsProvider *)
credentialsProvider // no passing ownership
userExecutor:(std::unique_ptr<firebase::firestore::util::Executor>)userExecutor
Expand Down
Loading