Track value category, staleness, and sync state separately for ActorCache::Entries #1033

bcaimano · 2023-08-18T16:20:45Z

This PR is paving the way for us to start coalescing reads/gets in the actor cache. The general plan looks like this:

Create an entry that admits it doesn't know what its value is (like we do for the end of list operations).
Give that entry some way to know that it is scheduled for a sync
Until that entry is replaced in the cache by one with a known value, any get requests that see that entry wait for a result to be available.

To that end, we're splitting the entry state into its three component parts:

Is the entry stale? If so, try to evict it eventually.
Is the entry present, absent, or unknown? If a get hits unknown, retrieve the value.
Is the entry dirty, flushing (synchronizing), or clean? If it is clean, we can sometimes avoid rpc altogether.

Sadly, this is a bit longer than I was hoping it would be. That said, the number of actual behavior changes should be relatively low. The biggest changes are around what is considered evictable (unknown entries now are) and how strict we are about tracking when something is in the cache.

jasnell · 2023-08-22T22:13:49Z

src/workerd/io/actor-cache.h

+        case EntrySyncStatus::NOT_IN_CACHE: {
+          KJ_FAIL_ASSERT("NOT_IN_CACHE entries should not be in the map or flushing");
+        }
+      }


Non-blocking nit: good idea to throw a KJ_UNREACHABLE after this switch?

MellowYarker

I've never read the actor-cache code before so I'd rather do a second pass in the morning before approving. Happy to see it get cleaned up, especially those switch statements!

src/workerd/io/actor-cache.c++

src/workerd/io/actor-cache.h

MellowYarker · 2023-08-22T22:07:47Z

src/workerd/io/actor-cache.c++

@@ -1414,7 +1409,6 @@ kj::Own<ActorCache::Entry> ActorCache::findInCache(
  if (iter != ordered.end() && iter->get()->key == key) {
    // Found exact matching entry.
    Entry& entry = **iter;
-    KJ_ASSERT(entry.state != NOT_IN_CACHE, "entry should have been removed from map");


I was a eyeing this assert but noticed it wasn't in the final diff so I'll ask here instead. Previously wondered the reasoning for checking NOT_IN_CACHE directly instead of UNKNOWN, but why delete this entirely?

I could leave it in if you prefer! Basically, this particular assert doesn't have anything to do with this function, we have no reason why we cannot return the entry since we pulled it from the map just now. I'd also be willing to add KJ_IASSERTS to all the places we set syncStatus to make sure the previous state is correct.

jasnell · 2023-08-25T20:24:48Z

doh, sorry, for some reason I thought this had already landed. I updated on the comment format which caused a merge conflict.

bcaimano · 2023-08-25T22:26:18Z

doh, sorry, for some reason I thought this had already landed. I updated on the comment format which caused a merge conflict.

No worries, thanks for updating the comment style in general!

…ache-test

This also fixes an inconsequential bug wherein we set dummy values to CLEAN instead of NOT_IN_CACHE.

Tracking these unknown values makes our logic around staleness and eviction simpler. It also prevents non-intersecting list calls (which add unknown entries for their end key) from filling up the cache with unevictable entries. Since both unknown (END_GAP) and known (CLEAN) entries can now be stale, entry staleness becomes its own boolean.

This commit introduces separate enum for if a value is present, absent, or unknown that is constant for the lifetime of an `Entry`. This makes our logic in several places clearer and provides the ground work for coalescing reads (since we'll need an entry in cache for them).

Since we have now moved staleness and value category to their own bytes, we can shrink down our sync status to its own byte and make it clearer that it indicates membership in various maps and lists.

…ctor-cache-no-state" This reverts commit 3a49006, reversing changes made to 812f78b.

…-no-state" This reverts commit 3a49006, reversing changes made to 812f78b.

Revert "Merge pull request #1033 from cloudflare/bcaimano/actor-cache-no-state

bcaimano changed the title ~~Convert ActorCache::Entry::state to track value category, staleness, and sync state separately~~ Track value category, staleness, and sync state separately for ActorCache::Entries Aug 18, 2023

bcaimano requested review from smerritt, jclee, jqmmes and MellowYarker August 18, 2023 16:55

bcaimano marked this pull request as ready for review August 18, 2023 16:55

jasnell reviewed Aug 22, 2023

View reviewed changes

MellowYarker reviewed Aug 22, 2023

View reviewed changes

bcaimano force-pushed the bcaimano/actor-cache-no-state branch from 658d4bb to 1f55c38 Compare August 23, 2023 20:47

MellowYarker approved these changes Aug 25, 2023

View reviewed changes

bcaimano force-pushed the bcaimano/actor-cache-no-state branch from 1f55c38 to 8582914 Compare August 28, 2023 22:54

bcaimano added 6 commits August 30, 2023 11:44

Refactor: Explicitly use multiples of expected entry sizes in actor-c…

7873c76

…ache-test

Cleanup: Use this instead of ActorCache::Entry::cache

200cc93

Cleanup: Simplify ActorCache::Entry creation

29b4b84

This also fixes an inconsequential bug wherein we set dummy values to CLEAN instead of NOT_IN_CACHE.

Cleanup: Rename EntryState to EntrySyncStatus

18d864c

Since we have now moved staleness and value category to their own bytes, we can shrink down our sync status to its own byte and make it clearer that it indicates membership in various maps and lists.

bcaimano force-pushed the bcaimano/actor-cache-no-state branch from 8582914 to 18d864c Compare August 30, 2023 15:44

bcaimano merged commit 3a49006 into main Aug 30, 2023
6 checks passed

bcaimano deleted the bcaimano/actor-cache-no-state branch August 30, 2023 19:18

jp4a50 added a commit to jp4a50/workerd that referenced this pull request Sep 8, 2023

Revert "Merge pull request cloudflare#1033 from cloudflare/bcaimano/a…

f91dda8

…ctor-cache-no-state" This reverts commit 3a49006, reversing changes made to 812f78b.

jp4a50 added a commit that referenced this pull request Sep 8, 2023

Revert "Merge pull request #1033 from cloudflare/bcaimano/actor-cache…

7492131

…-no-state" This reverts commit 3a49006, reversing changes made to 812f78b.

jp4a50 added a commit that referenced this pull request Sep 8, 2023

Revert "Merge pull request #1033 from cloudflare/bcaimano/actor-cache…

25d255f

…-no-state" This reverts commit 3a49006, reversing changes made to 812f78b.

jp4a50 added a commit that referenced this pull request Sep 8, 2023

Merge pull request #1155 from cloudflare/jphillips/revert-PR-1033-main

2182afd

Revert "Merge pull request #1033 from cloudflare/bcaimano/actor-cache-no-state

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track value category, staleness, and sync state separately for ActorCache::Entries #1033

Track value category, staleness, and sync state separately for ActorCache::Entries #1033

bcaimano commented Aug 18, 2023 •

edited

Loading

jasnell Aug 22, 2023

MellowYarker left a comment

MellowYarker Aug 22, 2023

bcaimano Aug 23, 2023

jasnell commented Aug 25, 2023

bcaimano commented Aug 25, 2023

Track value category, staleness, and sync state separately for ActorCache::Entries #1033

Track value category, staleness, and sync state separately for ActorCache::Entries #1033

Conversation

bcaimano commented Aug 18, 2023 • edited Loading

jasnell Aug 22, 2023

Choose a reason for hiding this comment

MellowYarker left a comment

Choose a reason for hiding this comment

MellowYarker Aug 22, 2023

Choose a reason for hiding this comment

bcaimano Aug 23, 2023

Choose a reason for hiding this comment

jasnell commented Aug 25, 2023

bcaimano commented Aug 25, 2023

bcaimano commented Aug 18, 2023 •

edited

Loading