Skip to content

Commit

Permalink
Refactor: Make EntrySyncStatus transitions clearer
Browse files Browse the repository at this point in the history
This commit introduces `addToCleanList()` and `addToDirtyList()` to make
it obvious when we're actually transitioning between lists.
  • Loading branch information
MellowYarker committed Mar 29, 2024
1 parent 8387801 commit 9f13fed
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 16 deletions.
24 changes: 8 additions & 16 deletions src/workerd/io/actor-cache.c++
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ void ActorCache::touchEntry(Lock& lock, Entry& entry) {
if (!entry.isDirty()) {
entry.isStale = false;
lock->remove(entry);
lock->add(entry);
addToCleanList(lock, entry);
}

// We only call `touchEntry` when the operation or the LRU has !noCache, so we want to cache this.
Expand Down Expand Up @@ -1489,8 +1489,7 @@ kj::Own<ActorCache::Entry> ActorCache::addReadResultToCache(
// the caching logic.
//
// Because of this, we know it is correct to leave `gapIsKnownEmpty = false` on our new entry.
entry->syncStatus = EntrySyncStatus::CLEAN;
lock->add(*entry);
addToCleanList(lock, *entry);
return kj::atomicAddRef(*entry);
});

Expand All @@ -1503,8 +1502,7 @@ kj::Own<ActorCache::Entry> ActorCache::addReadResultToCache(
KJ_ASSERT(!slot->gapIsKnownEmpty); // UNKNOWN entry should never have gapIsKnownEmpty.
removeEntry(lock, *slot);

entry->syncStatus = EntrySyncStatus::CLEAN;
lock->add(*entry);
addToCleanList(lock, *entry);
slot = kj::atomicAddRef(*entry);
break;
}
Expand Down Expand Up @@ -1597,8 +1595,7 @@ void ActorCache::markGapsEmpty(Lock& lock, KeyPtr beginKey, kj::Maybe<KeyPtr> en
// We must insert an UNKNOWN entry to cap our range.
KJ_IF_SOME(k, endKey) {
auto entry = kj::atomicRefcounted<Entry>(*this, cloneKey(k), EntryValueStatus::UNKNOWN);
entry->syncStatus = EntrySyncStatus::CLEAN;
lock->add(*entry);
addToCleanList(lock, *entry);
map.insert(kj::mv(entry));
} else {
// No UNKNOWN needed since the end is actually the end of the key space.
Expand Down Expand Up @@ -1981,8 +1978,7 @@ ActorCache::DeleteAllResults ActorCache::deleteAll(WriteOptions options) {
map.findOrCreate(Key{}, [&]() {
Key key;
auto entry = kj::atomicRefcounted<Entry>(*this, kj::mv(key), EntryValueStatus::ABSENT);
lock->add(*entry);
entry->syncStatus = EntrySyncStatus::CLEAN;
addToCleanList(lock, *entry);
entry->gapIsKnownEmpty = true;
return entry;
});
Expand Down Expand Up @@ -2085,9 +2081,7 @@ void ActorCache::putImpl(Lock& lock, kj::Own<Entry> newEntry,
// Swap in the new entry.
KJ_DASSERT(slot->key == newEntry->key);
slot = kj::mv(newEntry);
slot->syncStatus = EntrySyncStatus::DIRTY;
dirtyList.add(*slot);

addToDirtyList(*slot);
} else {
// No exact matching entry exists, insert a new one.

Expand All @@ -2111,8 +2105,7 @@ void ActorCache::putImpl(Lock& lock, kj::Own<Entry> newEntry,
KJ_IF_SOME(c, countedDelete) {
slot->countedDelete = kj::addRef(c);
}
slot->syncStatus = EntrySyncStatus::DIRTY;
dirtyList.add(*slot);
addToDirtyList(*slot);
}

ensureFlushScheduled(options);
Expand Down Expand Up @@ -2547,8 +2540,7 @@ kj::Promise<void> ActorCache::flushImpl(uint retryCount) {
}
}

entry.syncStatus = EntrySyncStatus::CLEAN;
lock->add(entry);
addToCleanList(lock, entry);
}
}
}
Expand Down
14 changes: 14 additions & 0 deletions src/workerd/io/actor-cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,20 @@ class ActorCache final: public ActorCacheInterface {
// Type of a lock on `SharedLru::cleanList`. We use the same lock to protect `currentValues`.
typedef kj::Locked<kj::List<Entry, &Entry::link>> Lock;

// Add this entry to the clean list and set its status to CLEAN.
// This doesn't do much, but it makes it easier to track what's going on.
void addToCleanList(Lock& listLock, Entry& entryRef) {
entryRef.syncStatus = EntrySyncStatus::CLEAN;
listLock->add(entryRef);
}

// Add this entry to the dirty list and set its status to DIRTY.
// This doesn't do much, but it makes it easier to track what's going on.
void addToDirtyList(Entry& entryRef) {
entryRef.syncStatus = EntrySyncStatus::DIRTY;
dirtyList.add(entryRef);
}

// Indicate that an entry was observed by a read operation and so should be moved to the end of
// the LRU queue.
void touchEntry(Lock& lock, Entry& entry);
Expand Down

0 comments on commit 9f13fed

Please sign in to comment.