Several caching improvements: #17930

mystenmark · 2024-05-24T23:57:42Z

Populate the cache with the latest object version when possible.
Add negative caching for getting object by id.
Use latest version cache for satisfying dynamic field reads

vercel · 2024-05-24T23:57:46Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 30, 2024 10:19pm

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	May 30, 2024 10:19pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	May 30, 2024 10:19pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	May 30, 2024 10:19pm

halfprice

LGTM with one comment about whether we are still able to read Deleted or Wrapped objects.

halfprice · 2024-05-29T18:20:18Z

crates/sui-core/src/execution_cache/writeback_cache.rs

+            if latest_object.is_live() {
+                return CacheResult::Hit((*latest_version, latest_object.clone()));
+            } else {
+                assert_eq!(*latest_version, SequenceNumber::from_u64(0));


Is this assertion true? We don't store deleted or wrapped version?

Also, I feel here is a semantic change. For deleted object, it was returning ObjectEntry::Deleted, but now it returns NegativeHit. Is this intended?

I thought NegativeHit means there is no such object version exists, including deleted/wrapped version.

cleaned this up

halfprice · 2024-05-29T18:57:09Z

crates/sui-core/src/execution_cache/writeback_cache.rs

+            // !is_fresh means we lost the race, and actual_entry holds the entry that was
+            // inserted by the other thread. We need to check if we have a more recent version
+            // than the other reader.
+            // This could also mean that the entry was inserted by a transaction write. This
+            // could occur in the following case:


I think this comment was a bit confusing. I though is_fresh means whether the entry is the latest entry in the cache or not. But after reading the doc, I realize that it basically means there was already an entry in the cache, which unnecessarily indicating a race.

I think the whole block basically does a test-and-set, isn't it? If so, updating the doc to something like "updating the object to latest version" may be clearer.

is_fresh means that our init callback ran, so !is_fresh means that we need to check if we have a more recent version that what is in the cache. I will try to clarify the comment

halfprice · 2024-05-29T18:57:36Z

crates/sui-core/src/execution_cache/writeback_cache.rs

+            //
+            // Thread 1 will see that v2 is already in the cache
+
+            // this point because there should have been a cache hit.)


Also, I'm not sure what does this sentence mean.

sorry, just an unfinished comment

halfprice · 2024-05-29T19:02:48Z

crates/sui-core/src/execution_cache/writeback_cache.rs

@@ -1060,9 +1153,7 @@ impl ObjectCacheRead for WritebackCache {
                ObjectEntry::Deleted => (object_id, version, ObjectDigest::OBJECT_DIGEST_DELETED),


Similar to above comment, can we still get ObjectEntry::Deleted or ObjectEntry::Wrapped here? Feels like it is going to return NegativeHit

Cleaned this up by having a different enum type for the latest-object cache

halfprice · 2024-05-29T19:10:18Z

crates/sui-core/src/execution_cache/writeback_cache.rs

+        if let Some(entry) = self.cached.object_by_id_cache.get(object_id) {
+            let entry = entry.lock();
+            let (latest_version, latest_object) = &*entry;
+            if latest_object.is_live() {


Ok, reading the PR again, I think I understand what is_live means, which basically try to accomodate cache_object_not_found. I think the criteria should be latest_version == 0 && entry == ObjectEntry::Deleted, right?

Or can we create an ObjectEntry::NotExist?

- Populate the cache with the latest object version when possible. - Add negative caching for getting object by id. - Use latest version cache for satisfying dynamic field reads

…bject

halfprice

LGTM! Thanks for adding all the detailed comments!

I wonder if it also worth creating some kind of cache invariant checker that runs periodically in simtest to catch issues. Some invariants may be that it could compare objects in dirty/cache/db and in object_by_id_cache, to see if they actually match.

halfprice · 2024-05-30T17:47:49Z

crates/sui-core/src/execution_cache/writeback_cache.rs

+    }
+
+    fn cache_object_not_found(&self, object_id: &ObjectID) {
+        // when caching non-existence, we use version 0. Since that is less than OBJECT_START_VERSION


Remove out dated comments?

mystenmark · 2024-05-30T20:47:33Z

LGTM! Thanks for adding all the detailed comments!

I wonder if it also worth creating some kind of cache invariant checker that runs periodically in simtest to catch issues. Some invariants may be that it could compare objects in dirty/cache/db and in object_by_id_cache, to see if they actually match.

adding a debug check

halfprice · 2024-05-30T22:06:59Z

LGTM! Thanks for adding all the detailed comments!
I wonder if it also worth creating some kind of cache invariant checker that runs periodically in simtest to catch issues. Some invariants may be that it could compare objects in dirty/cache/db and in object_by_id_cache, to see if they actually match.

adding a debug check

LGTM!

- Populate the cache with the latest object version when possible. - Add negative caching for getting object by id. - Use latest version cache for satisfying dynamic field reads

mystenmark requested review from lxfind and halfprice May 24, 2024 23:57

vercel bot deployed to Preview – sui-docs May 24, 2024 23:58 View deployment

vercel bot deployed to Preview – sui-docs May 29, 2024 17:45 View deployment

halfprice approved these changes May 29, 2024

View reviewed changes

mystenmark added 3 commits May 29, 2024 14:23

Several caching improvements:

b78b48d

- Populate the cache with the latest object version when possible. - Add negative caching for getting object by id. - Use latest version cache for satisfying dynamic field reads

Fix clear_and_assert_empty

dfaa739

Fixes for caching logic

8927d9e

mystenmark force-pushed the mlogan-cache-latest-object branch from 913f3f9 to 8927d9e Compare May 30, 2024 15:14

vercel bot deployed to Preview – sui-docs May 30, 2024 15:15 View deployment

Improve comments

f7b7a39

vercel bot deployed to Preview – sui-docs May 30, 2024 15:26 View deployment

Merge remote-tracking branch 'origin/main' into mlogan-cache-latest-o…

4c38fd2

…bject

mystenmark requested a review from halfprice May 30, 2024 17:22

vercel bot deployed to Preview – sui-docs May 30, 2024 17:22 View deployment

halfprice approved these changes May 30, 2024

View reviewed changes

mystenmark added 2 commits May 30, 2024 12:51

Add metrics for object_by_id_cache

3a99caf

Attempt to clarify comments

71fdafc

mystenmark force-pushed the mlogan-cache-latest-object branch from 92a0426 to 71fdafc Compare May 30, 2024 19:51

vercel bot deployed to Preview – sui-docs May 30, 2024 19:52 View deployment

remove outdated comment

01cf6cd

vercel bot deployed to Preview – sui-docs May 30, 2024 20:17 View deployment

mystenmark enabled auto-merge (squash) May 30, 2024 20:52

vercel bot deployed to Preview – sui-docs May 30, 2024 20:54 View deployment

mystenmark disabled auto-merge May 30, 2024 20:58

mystenmark enabled auto-merge (squash) May 30, 2024 22:02

Add coherency debug check for object_by_id_cache

dc0ab9d

mystenmark force-pushed the mlogan-cache-latest-object branch from 3908cb0 to dc0ab9d Compare May 30, 2024 22:03

vercel bot deployed to Preview – sui-docs May 30, 2024 22:05 View deployment

clippy

0180ef4

vercel bot deployed to Preview – sui-docs May 30, 2024 22:19 View deployment

mystenmark merged commit 563ae30 into main May 30, 2024
48 checks passed

mystenmark deleted the mlogan-cache-latest-object branch May 30, 2024 22:41

mystenmark mentioned this pull request Jun 6, 2024

experimental release w/ cache enabled #18102

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several caching improvements: #17930

Several caching improvements: #17930

mystenmark commented May 24, 2024

vercel bot commented May 24, 2024 •

edited

Loading

halfprice left a comment

halfprice May 29, 2024

mystenmark May 30, 2024

halfprice May 29, 2024

mystenmark May 30, 2024 •

edited

Loading

halfprice May 29, 2024

mystenmark May 30, 2024

halfprice May 29, 2024 •

edited

Loading

mystenmark May 30, 2024

halfprice May 29, 2024

halfprice left a comment

halfprice May 30, 2024

mystenmark commented May 30, 2024

halfprice commented May 30, 2024

		@@ -1060,9 +1153,7 @@ impl ObjectCacheRead for WritebackCache {
		ObjectEntry::Deleted => (object_id, version, ObjectDigest::OBJECT_DIGEST_DELETED),

Several caching improvements: #17930

Several caching improvements: #17930

Conversation

mystenmark commented May 24, 2024

vercel bot commented May 24, 2024 • edited Loading

halfprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mystenmark May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfprice May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mystenmark commented May 30, 2024

halfprice commented May 30, 2024

vercel bot commented May 24, 2024 •

edited

Loading

mystenmark May 30, 2024 •

edited

Loading

halfprice May 29, 2024 •

edited

Loading