-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Wire together LRU GC #1905
Conversation
LRU GC is currently hardcoded to be disabled at the point where it is initialized in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM with a last few nits.
return LruResults{NO, 0, 0, 0}; | ||
} | ||
|
||
BOOL didRun; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ has a boolean type bool
whose literals are true
and false
. (BOOL
, YES
, NO
are Objective-C).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, done.
using firebase::firestore::model::DocumentKey; | ||
using firebase::firestore::model::ListenSequenceNumber; | ||
|
||
const long kFIRFirestorePersistenceCacheSizeUnlimited = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this declaration should be up somewhere in API-land? Eventually this code will be platform neutral C++, while this constant has a name that's specific to Objective-C.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The declaration is in FIRFirestoreSettings.h
. Although, I have now set this to use a static constant from LruParams
so that when we have a C++ place to define the constant, we can set its value to the same internal constant.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we agree to make this CacheSizeUnlimited?
Also needs an NS_SWIFT_NAME
and a swift-language use test in BasicCompileTests.swift
to ensure that the name is mapped correctly out to swift users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, updated to kFIRFirestoreCacheSizeUnlimited
and int64_t
. The API review deferred to @ryanwilson for an swift naming convention, and he's out this week. I'll update it once I talk to him.
It's also now defined in terms of a value on LruParams
.
…because they were testing that GC didn't run
* Wire up LRU GC
* Add second timer setup, use NSInterval, drop post-compaction
Updated to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We also need a changelog entry but we can do that in a separate PR.
* Revert "Custom fdl domain (#2121)" This reverts commit ebea1ef. * Revert "enable disabled passed integration test (#2120)" This reverts commit 48e5f7a. * Revert "Wire together LRU GC (#1905)" This reverts commit 305f872. * Revert "Fix build under linux/gcc (#2116)" This reverts commit b0a4b6c. * Revert "Implement PatchMutation::ApplyToLocalView (#1973)" This reverts commit f66b5b5. * Revert "Add Firebase Source to Header Search Path (#2114)" This reverts commit 0540361.
This PR contains the public-facing API for LRU GC. It has approved by the API council and the required schema migration has already been merged.
Additionally, this PR wires up LRU garbage collection to run on a timer and log results. It implements both the timing and the size thresholds, as well as the method that executes the various pieces of the GC algorithm.