Skip to content

Commit

Permalink
Remove scope and collection cache (#512)
Browse files Browse the repository at this point in the history 
* Remove scope and collection cache

* Removed scope and collection cache in the CBLDatabase implementation. This change made CBL-C inline with the other platforms’ implementation. Plus, it makes the implementation much more simpler.

* CBLDatabase still caches the (internal) default collection used for default collection based APIs (Most are the deprecated APIs). Once we remove those deprecated API, we may not need to have it.

* When caching the internal default collection in the CBLDatabase, to prevent the retain cycle, call CBLCollection’s adopt(db) function to release the database instance in the CBLCollection object.

* The database instance inside CBLCollection and CBLScope will not be invalidated. This makes several part of the code that needs the database instance much simplier as it doesn’t need to take care the case when the database is null.

* Made changes to the tests as needed. Plus, fix XCode analyzer warning in QueryTest.cc about setting nullptr to listenerToken.

* Fixed wrong information about default collection in CBLCollection. (We will need to fix this in 3.1 branch as well).

* Updated LiteCore to 3.2.0-115 otherwise it’s not buildable on XCode 15. Need to fix LogTest as now the log files have two header lines.
  • Loading branch information
@pasin
pasin authored Dec 7, 2023
1 parent e915dad commit d1962c4
Show file tree
Hide file tree
Showing 18 changed files with 133 additions and 275 deletions.
4 changes: 1 addition & 3 deletions include/cbl/CBLCollection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,10 @@ CBLScope* CBLDatabase_DefaultScope(const CBLDatabase* db,
CBLError* _cbl_nullable outError) CBLAPI;

/** Returns the default collection.
@note The default collection may not exist if it was deleted.
Also, the default collection cannot be recreated after being deleted.
@note You are responsible for releasing the returned collection.
@param db The database.
@param outError On failure, the error will be written here.
@return A \ref CBLCollection instance, or NULL if the default collection doesn't exist or an error occurred. */
@return A \ref CBLCollection instance, or NULL if an error occurred. */
CBLCollection* _cbl_nullable CBLDatabase_DefaultCollection(const CBLDatabase* db,
CBLError* _cbl_nullable outError) CBLAPI;

Expand Down
14 changes: 2 additions & 12 deletions src/CBLCollection.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,18 +70,8 @@ namespace cbl_internal {
change.collection = _collection;
change.docID = _docID;

Retained<CBLDatabase> db;
try {
db = _collection->database();
} catch (...) {
C4Error error = C4Error::fromCurrentException();
CBL_Log(kCBLLogDomainDatabase, kCBLLogWarning,
"Document changed notification failed: %s", error.description().c_str());
}

if (db) {
db->notify(this, change);
}
auto db = _collection->database();
db->notify(this, change);
}

Retained<CBLCollection> _collection;
Expand Down
6 changes: 2 additions & 4 deletions src/CBLCollection_CAPI.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ CBLScope* CBLDatabase_DefaultScope(const CBLDatabase* db, CBLError* outError) no

CBLCollection* CBLDatabase_DefaultCollection(const CBLDatabase* db, CBLError* outError) noexcept {
try {
return const_cast<CBLDatabase*>(db)->getDefaultCollection(false).detach();
return const_cast<CBLDatabase*>(db)->getDefaultCollection().detach();
} catchAndBridge(outError)
}

Expand Down Expand Up @@ -115,9 +115,7 @@ uint64_t CBLCollection_Count(const CBLCollection* collection) noexcept {

/** Private API */
CBLDatabase* CBLCollection_Database(const CBLCollection* collection) noexcept {
try {
return collection->database();
} catchAndWarn()
return collection->database();
}

/** Private API */
Expand Down
65 changes: 29 additions & 36 deletions src/CBLCollection_Internal.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,17 @@ public:
CBLCollection(C4Collection* c4col, CBLScope* scope, CBLDatabase* database)
:_c4col(c4col, database)
,_scope(scope)
,_database(retain(database))
,_name(c4col->getName())
{ }

~CBLCollection() {
LOCK(_adoptMutex);
if (!_adopted) {
release(_database);
}
}

#pragma mark - ACCESSORS:

Retained<CBLScope> scope() const noexcept {return _scope;}
Expand All @@ -47,9 +55,7 @@ public:
bool isValid() const noexcept {return _c4col.isValid();}
uint64_t count() const {return _c4col.useLocked()->getDocumentCount();}
uint64_t lastSequence() const {return static_cast<uint64_t>(_c4col.useLocked()->getLastSequence());}

/** Throw NotOpen if the collection or database is invalid */
CBLDatabase* database() const {return _c4col.database();}
CBLDatabase* database() const {return _database; }

#pragma mark - DOCUMENTS:

Expand Down Expand Up @@ -161,17 +167,24 @@ protected:
friend struct CBLDocument;
friend struct cbl_internal::ListenerToken<CBLCollectionDocumentChangeListener>;

// Called by the database to take an ownership.
// Release the database to avoid the circular reference
void adopt(CBLDatabase* db) {
assert(_database == db);
LOCK(_adoptMutex);
if (!_adopted) {
_scope->adopt(db);
release(_database);
_adopted = true;
}
}

auto useLocked() { return _c4col.useLocked(); }
template <class LAMBDA>
void useLocked(LAMBDA callback) { _c4col.useLocked(callback); }
template <class RESULT, class LAMBDA>
RESULT useLocked(LAMBDA callback) { return _c4col.useLocked<RESULT>(callback); }

/** Called by the database when the database is released. */
void close() {
_c4col.close(); // This will invalidate the database pointer in the access lock
}

private:

Retained<CBLDocument> getDocument(slice docID, bool isMutable, bool allRevisions) const {
Expand Down Expand Up @@ -204,18 +217,7 @@ private:
}

void collectionChanged() {
Retained<CBLDatabase> db;
try {
db = database();
} catch (...) {
C4Error error = C4Error::fromCurrentException();
CBL_Log(kCBLLogDomainDatabase, kCBLLogWarning,
"Collection changed notification failed: %s", error.description().c_str());
}

if (db) {
db->notify(std::bind(&CBLCollection::callCollectionChangeListeners, this));
}
_database->notify(std::bind(&CBLCollection::callCollectionChangeListeners, this));
}

void callCollectionChangeListeners() {
Expand Down Expand Up @@ -253,12 +255,10 @@ private:
:shared_access_lock(std::move(c4col), *database->c4db())
,_c4db(database->c4db())
,_col(c4col)
,_db(database)
{
_sentry = [this](C4Collection* c4col) {
if (!_isValid()) {
C4Error::raise(LiteCoreDomain, kC4ErrorNotOpen,
"Invalid collection: either deleted, or db closed");
C4Error::raise(LiteCoreDomain, kC4ErrorNotOpen, "Invalid collection: either deleted or db closed");
}
};
}
Expand All @@ -268,22 +268,11 @@ private:
return _isValid();
}

CBLDatabase* database() const {
auto lock = useLocked();
return _db;
}

/** Invalidate the database pointer */
void close() noexcept {
LOCK_GUARD lock(getMutex());
_db = nullptr;
}

private:
bool _isValid() const noexcept { return _db && _col->isValid(); }
// Unsafe: need to call under lock:
bool _isValid() const noexcept { return !_c4db->isClosedNoLock() && _col->isValid(); }

CBLDatabase::SharedC4DatabaseAccessLock _c4db; // For retaining the shared lock
CBLDatabase* _cbl_nullable _db;
C4Collection* _col;
};

Expand All @@ -294,6 +283,10 @@ private:
alloc_slice _name;
Retained<CBLScope> _scope;

CBLDatabase* _database; // Retained unless being adopted
bool _adopted {false}; // Adopted by the database
mutable std::mutex _adoptMutex;

std::unique_ptr<C4CollectionObserver> _observer;
Listeners<CBLCollectionChangeListener> _listeners;
Listeners<CBLCollectionDocumentChangeListener> _docListeners;
Expand Down
130 changes: 14 additions & 116 deletions src/CBLDatabase.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ CBLDatabase::CBLDatabase(C4Database* _cbl_nonnull db, slice name_, slice dir_)
,_notificationQueue(this)
{
_c4db = std::make_shared<C4DatabaseAccessLock>(db);
_defaultCollection = getCollection(kC4DefaultCollectionName, kC4DefaultScopeID);
}


Expand Down Expand Up @@ -129,14 +128,6 @@ void CBLDatabase::closeAndDelete() {

/** Must called under _c4db lock. */
void CBLDatabase::_closed() {
// Close scopes:
for (auto& i : _scopes) {
i.second->close();
}
// Close collections:
for (auto& i : _collections) {
i.second->close();
}
// Close the access lock:
_c4db->close();
}
Expand All @@ -151,34 +142,11 @@ Retained<CBLScope> CBLDatabase::getScope(slice scopeName) {

auto c4db = _c4db->useLocked();

CBLScope* scope = nullptr;

bool exist = c4db->hasScope(scopeName);
if (auto i = _scopes.find(scopeName); i != _scopes.end()) {
if (!exist) {
// Detach instead of close so that the retained scope
// object can retain the database and can be valid to use.
i->second->detach();

// Remove from the _scopes map.
_scopes.erase(i);

// Remove all collections in the scope from the _collections map:
// This is techically required to prevent circular reference
// (db->collection->scope->db) when the scope is detached.
removeCBLCollections(scopeName);
return nullptr;
}
scope = i->second.get();
}

if (!scope && exist) {
auto retainedScope = make_retained<CBLScope>(scopeName, this);
scope = retainedScope.get();
_scopes.insert({scope->name(), std::move(retainedScope)});
if (!exist) {
return nullptr;
}

return scope;
return new CBLScope(scopeName, this);
}


Expand All @@ -191,33 +159,13 @@ Retained<CBLCollection> CBLDatabase::getCollection(slice collectionName, slice s

auto c4db = _c4db->useLocked();

CBLCollection* collection = nullptr;
auto spec = C4Database::CollectionSpec(collectionName, scopeName);
if (auto i = _collections.find(spec); i != _collections.end()) {
collection = i->second.get();
}

if (collection && collection->isValid()) {
return collection;
}

auto c4col = c4db->getCollection(spec);
if (!c4col) {
if (collection) {
removeCBLCollection(spec); // Invalidate cache
}
return nullptr;
}

auto scope = getScope(scopeName);
if (!scope) {
// Note (Edge Case):
// The scope is NULL because at the same time, its all collections including the
// the one just created were deleted on a different thread using another database.
return nullptr;
}

return createCBLCollection(c4col, scope.get());
auto scope = new CBLScope(scopeName, this);
return new CBLCollection(c4col, scope, this);
}


Expand All @@ -227,26 +175,10 @@ Retained<CBLCollection> CBLDatabase::createCollection(slice collectionName, slic

auto c4db = _c4db->useLocked();

auto col = getCollection(collectionName, scopeName);
if (col) {
return col;
}

auto spec = C4Database::CollectionSpec(collectionName, scopeName);
auto c4col = c4db->createCollection(spec);

bool cache = true;
auto scope = getScope(scopeName);
if (!scope) {
// Note (Edge Case):
// The scope is NULL because at the same time, its all collections including the
// the one just created were deleted on a different thread using another database.
// So create the scope in non-cached mode, and the returned collection will not
// be cached.
cache = false;
scope = new CBLScope(scopeName, this, cache);
}
return createCBLCollection(c4col, scope.get(), cache);
auto scope = new CBLScope(scopeName, this);
return new CBLCollection(c4col, scope, this);
}


Expand All @@ -258,60 +190,26 @@ bool CBLDatabase::deleteCollection(slice collectionName, slice scopeName) {

auto spec = C4Database::CollectionSpec(collectionName, scopeName);
c4db->deleteCollection(spec);
removeCBLCollection(spec);
return true;
}


Retained<CBLScope> CBLDatabase::getDefaultScope() {
_c4db->useLocked();
return getScope(kC4DefaultScopeID);
}


Retained<CBLCollection> CBLDatabase::getDefaultCollection(bool mustExist) {
auto db = _c4db->useLocked();

if (_defaultCollection && !_defaultCollection->isValid()) {
_defaultCollection = nullptr;
}

if (!_defaultCollection && mustExist) {
C4Error::raise(LiteCoreDomain, kC4ErrorNotOpen,
"Invalid collection: either deleted, or db closed");
}

return _defaultCollection;
Retained<CBLCollection> CBLDatabase::getDefaultCollection() {
return getCollection(kC4DefaultCollectionName, kC4DefaultScopeID);
}


Retained<CBLCollection> CBLDatabase::createCBLCollection(C4Collection* c4col, CBLScope* scope, bool cache) {
auto retainedCollection = make_retained<CBLCollection>(c4col, scope, const_cast<CBLDatabase*>(this));
auto collection = retainedCollection.get();
if (cache) {
_collections.insert({C4Database::CollectionSpec(c4col->getSpec()), std::move(retainedCollection)});
}
return collection;
}


void CBLDatabase::removeCBLCollection(C4Database::CollectionSpec spec) {
if (auto i = _collections.find(spec); i != _collections.end()) {
i->second.get()->close();
_collections.erase(i);
}
}

void CBLDatabase::removeCBLCollections(slice scopeName) {
auto i = _collections.begin();
while (i != _collections.end()) {
if (i->first.scope == scopeName) {
i->second->close();
i = _collections.erase(i);
} else {
i++;
}
Retained<CBLCollection> CBLDatabase::getInternalDefaultCollection() {
if (!_defaultCollection) {
_defaultCollection = getCollection(kC4DefaultCollectionName, kC4DefaultScopeID);
_defaultCollection->adopt(this); // Prevent the retain cycle
}
return _defaultCollection;
}


Expand Down
Loading

0 comments on commit d1962c4

Please sign in to comment.