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.

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

New Cache Config and LRU GC #1308

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

New Cache Config and LRU GC #1308

wants to merge 25 commits into from

Conversation

wu-hui
Copy link
Contributor

@wu-hui wu-hui commented May 8, 2023

First half of the implementation, Android implementation will be a separate PR which will get merged into this one before everything is merged into main.

@wu-hui wu-hui added the tests-requested: quick Trigger a quick set of integration tests. label May 8, 2023
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label May 8, 2023
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

I didn't do a thorough review of everything, but I see major issues in the design of the "include" files. Those files must be platform-independent and cannot reference stuff in the ios sdk because they are used on Android as well. Also, the static_cast is troublesome to me because in principle we should be able to use the class hierarchy to avoid casting. Otherwise, there isn't much point to having a common base class IMO.

firestore/integration_test_internal/src/firestore_test.cc Outdated Show resolved Hide resolved
firestore/integration_test_internal/src/firestore_test.cc Outdated Show resolved Hide resolved
firestore/integration_test_internal/src/firestore_test.cc Outdated Show resolved Hide resolved
firestore/src/common/local_cache_settings.cc Outdated Show resolved Hide resolved
firestore/src/common/settings.cc Show resolved Hide resolved
firestore/src/main/firestore_main.cc Outdated Show resolved Hide resolved
@dconeybe dconeybe assigned wu-hui and unassigned dconeybe May 11, 2023
Copy link
Contributor Author

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

Thanks for the review!

firestore/integration_test_internal/src/firestore_test.cc Outdated Show resolved Hide resolved
firestore/integration_test_internal/src/firestore_test.cc Outdated Show resolved Hide resolved
firestore/integration_test_internal/src/firestore_test.cc Outdated Show resolved Hide resolved
firestore/src/common/local_cache_settings.cc Outdated Show resolved Hide resolved
firestore/src/main/firestore_main.cc Outdated Show resolved Hide resolved
firestore/src/common/settings.cc Show resolved Hide resolved
@wu-hui wu-hui assigned dconeybe and unassigned wu-hui May 15, 2023
@wu-hui
Copy link
Contributor Author

wu-hui commented Jun 2, 2023

@dconeybe I took some nice stuff from your prototype and added to this PR, but I decided to keep the enum/static_cast around.

I did try to use absl::variant and Impl classes, and I do not quite like the result, mostly because in the end the approved api does not work well. I also think they add too much complexity for a small gain. enum/static_cast are very well understood and used in our code base in many places.

Please let me know your thoughts, we can discuss this further.

@dconeybe
Copy link
Contributor

dconeybe commented Jun 2, 2023

I took a quick look and left some minor comments. I'll do a more holistic review later today or early next week.

@dconeybe dconeybe assigned wu-hui and unassigned dconeybe Jun 5, 2023
@wu-hui wu-hui assigned dconeybe and unassigned wu-hui Jun 7, 2023
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

I'm still concerned about the "main" API bleeding into the public header file. I don't see how this will work on Android. Please correct me if I'm wrong about that. Other than that, I've just left a few comments. I'll do a deeper review once I feel that the API will work for both Android and main.

* Returns a reference to the `LocalCacheSettings` instance
* used to configure this SDK.
*/
const LocalCacheSettings& local_cache_settings();
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider returning by value (i.e. returning a copy) from local_cache_settings() since the lifetime of the returned reference is... complicated. Specifically, the returned reference becomes invalid if there is a subsequent call to set_local_cache_settings() or set_persistence_enabled() or a few other functions. IMO in this particular instance the clarity and memory safety of returning by value far outweighs any performance benefit of returning by reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with what you said, but I don't think it's possible to return a copy of LocalCacheSetting because it is abstract?

We could consider returning the underlying shared_ptr though..this is not in the approved proposal but seems like a good amendment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm fair enough. Maybe returning a const reference is fine. Consider documenting, though, the situations under which the returned reference becomes invalid.

@dconeybe dconeybe assigned wu-hui and unassigned dconeybe Jun 7, 2023
Copy link
Contributor Author

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

Thanks! I have to admit I did not think about how to implement Android enough!

* Returns a reference to the `LocalCacheSettings` instance
* used to configure this SDK.
*/
const LocalCacheSettings& local_cache_settings();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with what you said, but I don't think it's possible to return a copy of LocalCacheSetting because it is abstract?

We could consider returning the underlying shared_ptr though..this is not in the approved proposal but seems like a good amendment?

@wu-hui wu-hui assigned dconeybe and unassigned wu-hui Jun 7, 2023
@dconeybe dconeybe assigned wu-hui and unassigned dconeybe Jun 12, 2023
@wu-hui wu-hui assigned dconeybe and unassigned wu-hui Jun 13, 2023
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

local_cache_settings.h still has an include line for local_cache_settings_main.h. local_cache_settings.h must not rely on anything specific to the ios implementation, since the same .h file must be used for Android.

@dconeybe dconeybe assigned wu-hui and unassigned dconeybe Jun 13, 2023
*/
class LocalCacheSettings {
public:
enum class Kind { kMemory, kPersistent };

Choose a reason for hiding this comment

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

📝 Documentation issue: warning: Member Kind (enumeration) of class firebase::firestore::LocalCacheSettings is not documented.


virtual ~LocalCacheSettings() = default;

virtual Kind kind() const = 0;

Choose a reason for hiding this comment

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

📝 Documentation issue: warning: Member kind() const =0 (function) of class firebase::firestore::LocalCacheSettings is not documented.

Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

I think you got the "core" dependencies out of the public headers. Nice job! I've reviewed the code, but haven't looked deeply at the tests. I'll look at the tests in the next round of review.


PersistentCacheSettings();

LocalCacheSettings::Kind kind() const override {
Copy link
Contributor

Choose a reason for hiding this comment

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

kind() should be public, since it's public in the base class LocalCacheSettings. Alternately, kind() could be made protected in the base class, along with the enum class Kind in the base class. In any case, they should be consistent. Was kind() part of the API proposal?

This comment also applies to MemoryCacheSettings::kind().

return LocalCacheSettings::Kind::kPersistent;
}

// Get the corresponding settings object from the core sdk.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "core sdk" -> "underlying sdk" or something like that, since it could be the "Android SDK", not the "Core SDK". You can also just omit this comment IMO since it's pretty obvious from the name what the function is doing.

}

// Get the corresponding settings object from the core sdk.
const LocalCacheSettingsInternal& internal() const override;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the compiler allow you to specialize the return type to const PersistentCacheSettingsInternal&?

const MemoryCacheSettings& rhs);

/**
* Copies this settings instance, with its `MemoryGarbageCollectorSettins` set
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: "MemoryGarbageCollectorSettins"

const MemoryCacheSettings& rhs);

/**
* Copies this settings instance, with its `MemoryGarbageCollectorSettins` set
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "set" -> "set to"

return false;
}

if (*lhs.local_cache_settings_ != *rhs.local_cache_settings_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if lhs and rhs have different types of persistence? For example, what if *lhs.local_cache_settings_ is memory local cache and *rhs.local_cache_settings_ is persistent local cache? Which operator== is used to compare them? I'm actually surprised this compiles, and doesn't crash if they are different types.

settings.set_persistence_enabled(from.is_persistence_enabled());
settings.set_cache_size_bytes(from.cache_size_bytes());
// TODO(wuandy): Checking `from.local_cache_settings_` is required, because
// FirestoreInternal::settings() overrides used_legacy_cache_settings_. All
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "used_legacy_cache_settings_"? I think maybe the name has changed. Please update this comment.

@@ -48,6 +48,7 @@ class Firestore;
class ListenerRegistrationInternal;
class Transaction;
class WriteBatch;
class Settings;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Put the forward declaration of Settings in its correct alphabetical ordering among the other forward declarations.

public:
friend bool operator==(const LocalCacheSettingsInternal& lhs,
const LocalCacheSettingsInternal& rhs) {
return &lhs == &rhs || lhs.core_settings() == rhs.core_settings();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this checking for &lhs == &rhs strictly required? If no, I recommend removing it since it is a performance optimization that likely is not warranted. Applies here and elsewhere in this file.

return &lhs == &rhs || lhs.settings_ == rhs.settings_;
}

PersistentCacheSettingsInternal WithSizeBytes(int64_t size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can WithSizeBytes() be const (as well as other "With" member functions declared/defined in this file)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests: in-progress This PR's integration tests are in progress. tests-requested: quick Trigger a quick set of integration tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants