[move] Remove compiled scripts from data cache #14026

georgemitenkov · 2024-07-17T09:33:44Z

Description

The new code cache caches deserialization, which means transaction data cache should not be used for any kind of module loading. This PR changes existing script cache in loader so that deserialized scripts can be cached there directly. It also allows us to cache deserialized scripts across multiple sessions & transactions.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-07-17T09:33:47Z

⏱️ 1h 11m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
test-fuzzers	37m	🟩
rust-move-tests	15m	🟩
rust-move-unit-coverage	13m	🟩
general-lints	2m	🟩
rust-cargo-deny	2m	🟩
check-dynamic-deps	1m	🟩
semgrep/ci	24s	🟩
file_change_determinator	11s	🟩
file_change_determinator	11s	🟩
permission-check	3s	🟩
permission-check	3s	🟩
permission-check	2s	🟩
permission-check	2s	🟩

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

codecov · 2024-07-17T09:45:23Z

Codecov Report

Attention: Patch coverage is 62.66667% with 28 lines in your changes missing coverage. Please review.

Project coverage is 58.9%. Comparing base (235402e) to head (97f1055).
Report is 5 commits behind head on main.

Files	Patch %	Lines
third_party/move/move-vm/runtime/src/loader/mod.rs	66.6%	16 Missing ⚠️
...rd_party/move/move-vm/runtime/src/loader/script.rs	47.8%	12 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #14026       +/-   ##
===========================================
- Coverage    70.8%    58.9%    -11.9%     
===========================================
  Files        2324      824     -1500     
  Lines      459502   198261   -261241     
===========================================
- Hits       325634   116938   -208696     
+ Misses     133868    81323    -52545

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gelash · 2024-07-20T17:33:49Z

third_party/move/move-vm/runtime/src/loader/mod.rs

 // Access to this cache is always under a `RwLock`.
 #[derive(Clone)]
 pub(crate) struct BinaryCache<K, V> {
    // Notice that we are using the HashMap implementation from the hashbrown crate, not the
    // one from std, as it allows alternative key representations to be used for lookup,
    // making certain optimizations possible.
    id_map: hashbrown::HashMap<K, usize>,
-    binaries: Vec<Arc<V>>,


Many such instances, thanks!
whenever ownership is clear and outlives the uses, we should use references (unless lifetime metaprogramming really gets out of hand, and I think our threshold has been low there)

gelash · 2024-07-20T17:39:47Z

third_party/move/move-vm/runtime/src/loader/script.rs

+        compiled_script: CompiledScript,
+    ) -> Arc<CompiledScript> {
+        let compiled_script_to_return = Arc::new(compiled_script);
+        self.scripts.insert(


What happens if we are inserting at a place that already contains a verified script? should that be allowed, no-op, or not expected from caller? If we introduce any logic let's also unit test.

gelash · 2024-07-20T17:46:05Z

third_party/move/move-vm/runtime/src/loader/mod.rs

@@ -269,7 +269,15 @@ impl Loader {
        sha3_256.update(script_blob);
        let hash_value: [u8; 32] = sha3_256.finalize().into();

-        let script = data_store.load_compiled_script_to_cache(script_blob, hash_value)?;
+        let mut scripts = self.scripts.write();


we could use a read lock, then upgrade to write lock if we need to insert (can find example in scheduler), but I'd imagine it doesn't matter here. However, .entry function might be slightly better here as it would make it clear in this case we are just replacing empty slot with a deserialized/compiled script. See the comment below, otherwise APIs seem more general (for the data-structure, not entry level), e.g. insert_compiled_script which would have to deal with observing an existing entry?

seems interesting if the designer didn't intend it under a lock (and makes sense to not be under a lock) - but also does scripts need to be a DashMap or something more granular than a global RW lock?

another question: probably the below logic, and also similar logic in load_script could be good separate utility methods - will also give more space to e.g. improve implementation with locks, etc.

gelash · 2024-07-20T17:47:43Z

third_party/move/move-vm/runtime/src/loader/mod.rs

-                    script_blob,
-                    hash_value,
-                    data_store,
+                let compiled_script = self.deserialize_script(script_blob)?;


First, would insert deserialized, then re-use the handling for verification (deserialized to verified). Makes me think that these could become the APIs of the script cache- e.g. if we never insert verified directly.

gelash · 2024-07-21T14:57:09Z

third_party/move/move-vm/runtime/src/loader/mod.rs

+        match entry {
+            ScriptCacheEntry::Verified(script) => script.clone(),
+            ScriptCacheEntry::Deserialized(_) => {
+                unreachable!("Script must be verified before it is main function scope is accessed")


nit: should "it is" be "its"?

gelash · 2024-07-21T15:01:11Z

third_party/move/move-vm/runtime/src/loader/mod.rs

-                    hash_value,
-                    data_store,
+                let compiled_script = self.deserialize_script(script_blob)?;
+                self.verify_script(&compiled_script, data_store, module_store)?;


hm, the comment on verify_script (previously on deserialize_and_verify) said it should not happen under a lock, but as far as I can see, it happens under scripts write lock? should we give up and re-acquire?

gelash · 2024-07-21T15:05:45Z

third_party/move/move-vm/runtime/src/loader/mod.rs

@@ -269,7 +269,15 @@ impl Loader {
        sha3_256.update(script_blob);
        let hash_value: [u8; 32] = sha3_256.finalize().into();

-        let script = data_store.load_compiled_script_to_cache(script_blob, hash_value)?;
+        let mut scripts = self.scripts.write();


seems interesting if the designer didn't intend it under a lock (and makes sense to not be under a lock) - but also does scripts need to be a DashMap or something more granular than a global RW lock?

another question: probably the below logic, and also similar logic in load_script could be good separate utility methods - will also give more space to e.g. improve implementation with locks, etc.

[move] Remove compiled scripts from data cache

97f1055

georgemitenkov requested review from runtian-zhou, vgao1996, gelash and ziaptos July 17, 2024 09:33

georgemitenkov changed the base branch from main to george/module-mv-storage July 17, 2024 19:08

georgemitenkov requested review from msmouse, lightmark, grao1991, a team, bchocho, sasha8, zekun000, ibalajiarun, JoshLind and gregnazario as code owners July 17, 2024 19:08

georgemitenkov changed the base branch from george/module-mv-storage to main July 17, 2024 19:11

georgemitenkov removed request for a team, gregnazario, bchocho, lightmark, msmouse, ibalajiarun, grao1991, JoshLind, sasha8 and zekun000 July 17, 2024 19:11

gelash reviewed Jul 20, 2024

View reviewed changes

gelash requested changes Jul 21, 2024

View reviewed changes

georgemitenkov closed this Aug 11, 2024

georgemitenkov deleted the george/script-deserialization-caching branch August 11, 2024 01:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move] Remove compiled scripts from data cache #14026

[move] Remove compiled scripts from data cache #14026

georgemitenkov commented Jul 17, 2024 •

edited

Loading

trunk-io bot commented Jul 17, 2024 •

edited

Loading

codecov bot commented Jul 17, 2024

gelash Jul 20, 2024

gelash Jul 20, 2024

gelash Jul 20, 2024

gelash Jul 21, 2024

gelash Jul 20, 2024

gelash Jul 21, 2024

gelash Jul 21, 2024

gelash Jul 21, 2024

[move] Remove compiled scripts from data cache #14026

[move] Remove compiled scripts from data cache #14026

Conversation

georgemitenkov commented Jul 17, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Jul 17, 2024 • edited Loading

codecov bot commented Jul 17, 2024

Codecov Report

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

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

georgemitenkov commented Jul 17, 2024 •

edited

Loading

trunk-io bot commented Jul 17, 2024 •

edited

Loading