Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make disabling GC prevent even scheduling it #2862

Merged
merged 4 commits into from
Apr 19, 2019
Merged

Make disabling GC prevent even scheduling it #2862

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2f49c0
Make disabling GC prevent even scheduling it
wilhuff Apr 18, 2019
a299a8a
Migrate settings defaults to C++ settings
wilhuff Apr 18, 2019
c70fdb7
Revert changes to integration testing.
wilhuff Apr 18, 2019
8b7d9c0
style.sh generated changes
wilhuff Apr 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 9 additions & 6 deletions Firestore/Source/API/FIRFirestoreSettings.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@

NS_ASSUME_NONNULL_BEGIN

using firebase::firestore::api::Settings;
using firebase::firestore::api::ThrowInvalidArgument;
using firebase::firestore::util::MakeString;
namespace api = firebase::firestore::api;
namespace util = firebase::firestore::util;
using api::ThrowInvalidArgument;

static NSString *const kDefaultHost = @"firestore.googleapis.com";
static const BOOL kDefaultSSLEnabled = YES;
Expand All @@ -35,6 +35,9 @@
static const int64_t kMinimumCacheSizeBytes = 1 * 1024 * 1024;
static const BOOL kDefaultTimestampsInSnapshotsEnabled = YES;

// Public constant
extern "C" const int64_t kFIRFirestoreCacheSizeUnlimited = api::Settings::CacheSizeUnlimited;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I did the api::Settings port I was thinking about moving all the defaults to be defined there but wasn't sure if that would open the door for static initialization order fiasco, so I left it as-is. But I guess this is safe because it's a builtin value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key to making that work is to ensure that the values are compiler initialized. Making values constexpr is one guaranteed way to do that. Another is to annotate the value with ABSL_CONST_INIT which adds an attribute that would cause a compiler error if the value could not be compiler initialized.

With that in place it's safe to refer to other values in the definition of a constant. I've ported all the settings defaults as well.


@implementation FIRFirestoreSettings

- (instancetype)init {
Expand Down Expand Up @@ -119,9 +122,9 @@ - (void)setCacheSizeBytes:(int64_t)cacheSizeBytes {
_cacheSizeBytes = cacheSizeBytes;
}

- (Settings)internalSettings {
Settings settings;
settings.set_host(MakeString(_host));
- (api::Settings)internalSettings {
api::Settings settings;
settings.set_host(util::MakeString(_host));
settings.set_ssl_enabled(_sslEnabled);
settings.set_persistence_enabled(_persistenceEnabled);
settings.set_timestamps_in_snapshots_enabled(_timestampsInSnapshotsEnabled);
Expand Down
4 changes: 3 additions & 1 deletion Firestore/Source/Core/FSTFirestoreClient.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,9 @@ - (void)initializeWithUser:(const User &)user settings:(const Settings &)setting
}
_lruDelegate = ldb.referenceDelegate;
_persistence = ldb;
[self scheduleLruGarbageCollection];
if (settings.gc_enabled()) {
[self scheduleLruGarbageCollection];
}
} else {
_persistence = [FSTMemoryPersistence persistenceWithEagerGC];
}
Expand Down
6 changes: 2 additions & 4 deletions Firestore/Source/Local/FSTLRUGarbageCollector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
#include <string>
#include <unordered_map>

#import "FIRFirestoreSettings.h"
#import "Firestore/Source/Local/FSTQueryData.h"

#include "Firestore/core/src/firebase/firestore/api/settings.h"
#include "Firestore/core/src/firebase/firestore/local/query_cache.h"
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
#include "Firestore/core/src/firebase/firestore/model/types.h"
Expand All @@ -37,14 +37,12 @@ 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{kFIRFirestoreCacheSizeUnlimited, 0, 0};
return LruParams{api::Settings::CacheSizeUnlimited, 0, 0};
}

static LruParams WithCacheSize(int64_t cacheSize) {
Expand Down
9 changes: 7 additions & 2 deletions Firestore/Source/Local/FSTLRUGarbageCollector.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@

#import "Firestore/Source/Local/FSTPersistence.h"
#include "Firestore/core/include/firebase/firestore/timestamp.h"
#include "Firestore/core/src/firebase/firestore/api/settings.h"
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
#include "Firestore/core/src/firebase/firestore/util/log.h"

namespace api = firebase::firestore::api;
using Millis = std::chrono::milliseconds;
using firebase::Timestamp;
using firebase::firestore::local::LruParams;
Expand All @@ -33,13 +35,14 @@
using firebase::firestore::model::ListenSequenceNumber;
using firebase::firestore::model::TargetId;

const int64_t kFIRFirestoreCacheSizeUnlimited = LruParams::CacheSizeUnlimited;
const ListenSequenceNumber kFSTListenSequenceNumberInvalid = -1;

static Millis::rep millisecondsBetween(const Timestamp &start, const Timestamp &end) {
return std::chrono::duration_cast<Millis>(end.ToTimePoint() - start.ToTimePoint()).count();
}

namespace {

/**
* RollingSequenceNumberBuffer tracks the nth sequence number in a series. Sequence numbers may be
* added out of order.
Expand Down Expand Up @@ -79,6 +82,8 @@ size_t size() const {
const size_t max_elements_;
};

} // namespace

@implementation FSTLRUGarbageCollector {
__weak id<FSTLRUDelegate> _delegate;
LruParams _params;
Expand All @@ -95,7 +100,7 @@ - (instancetype)initWithDelegate:(id<FSTLRUDelegate>)delegate params:(LruParams)

- (LruResults)collectWithLiveTargets:
(const std::unordered_map<TargetId, FSTQueryData *> &)liveTargets {
if (_params.minBytesThreshold == kFIRFirestoreCacheSizeUnlimited) {
if (_params.minBytesThreshold == api::Settings::CacheSizeUnlimited) {
LOG_DEBUG("Garbage collection skipped; disabled");
return LruResults::DidNotRun();
}
Expand Down
3 changes: 2 additions & 1 deletion Firestore/Source/Public/FIRFirestoreSettings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
NS_ASSUME_NONNULL_BEGIN

/** Used to set on-disk cache size to unlimited. Garbage collection will not run. */
extern const int64_t kFIRFirestoreCacheSizeUnlimited NS_SWIFT_NAME(FirestoreCacheSizeUnlimited);
FOUNDATION_EXTERN const int64_t kFIRFirestoreCacheSizeUnlimited
NS_SWIFT_NAME(FirestoreCacheSizeUnlimited);

/** Settings used to configure a `FIRFirestore` instance. */
NS_SWIFT_NAME(FirestoreSettings)
Expand Down
5 changes: 5 additions & 0 deletions Firestore/core/src/firebase/firestore/api/settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ namespace api {
*/
class Settings {
public:
static const int64_t CacheSizeUnlimited = -1;

Settings() = default;

void set_host(const std::string& value) {
Expand Down Expand Up @@ -68,6 +70,9 @@ class Settings {
int64_t cache_size_bytes() const {
return cache_size_bytes_;
}
bool gc_enabled() const {
return cache_size_bytes_ != CacheSizeUnlimited;
}

friend bool operator==(const Settings& lhs, const Settings& rhs);

Expand Down