feat: prefetch more trie keys #10899

Longarithm · 2024-03-28T20:25:45Z

No description provided.

mooori · 2024-03-29T10:42:40Z

Could the keys accessed via ft_balance_of be prefetched as well? Such access was quite frequent in Cosmose unit tests, though I don't know how relevant they are for mainnet traffic.

Longarithm · 2024-03-29T10:45:30Z

Ah. I suspect ft_balance_of is a read-only query, and these ones are served from FlatStorage, which is only 1 fast disk read. Only write queries matter.

Note: with stateless validation everything will be served from memtrie in memory, so hopefully this hack is a short-term. We consider enabling memtries even sooner.

codecov · 2024-03-29T12:14:46Z

Codecov Report

Attention: Patch coverage is 42.04545% with 102 lines in your changes are missing coverage. Please review.

Project coverage is 71.44%. Comparing base (57ff810) to head (964bb71).

Files	Patch %	Lines
runtime/runtime/src/prefetch.rs	31.75%	93 Missing and 8 partials ⚠️
core/store/src/trie/config.rs	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10899      +/-   ##
==========================================
- Coverage   71.49%   71.44%   -0.06%     
==========================================
  Files         759      759              
  Lines      152522   152594      +72     
  Branches   152522   152594      +72     
==========================================
- Hits       109042   109015      -27     
- Misses      38964    39051      +87     
- Partials     4516     4528      +12

Flag	Coverage Δ
backward-compatibility	`0.24% <0.00%> (-0.01%)`	⬇️
db-migration	`0.24% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.43% <5.74%> (+<0.01%)`	⬆️
integration-tests	`37.12% <21.02%> (-0.06%)`	⬇️
linux	`69.92% <41.47%> (-0.02%)`	⬇️
linux-nightly	`70.89% <42.04%> (-0.07%)`	⬇️
macos	`54.46% <32.95%> (-0.04%)`	⬇️
pytests	`1.65% <5.74%> (+<0.01%)`	⬆️
sanity-checks	`1.44% <5.74%> (+<0.01%)`	⬆️
unittests	`67.07% <41.47%> (-0.05%)`	⬇️
upgradability	`0.29% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

wacban

Just nits from my side. I'm not familiar with the prefetch so I'll leave it to @mooori to review and I'll stamp when his review is done.

wacban · 2024-04-02T07:37:22Z

core/store/src/trie/shard_tries.rs

            && (self.0.trie_config.enable_receipt_prefetching
                || (!self.0.trie_config.sweat_prefetch_receivers.is_empty()
-                    && !self.0.trie_config.sweat_prefetch_senders.is_empty()));
+                    && !self.0.trie_config.sweat_prefetch_senders.is_empty())
+                || !self.0.trie_config.claim_sweat_prefetch_config.is_empty()
+                || !self.0.trie_config.kaiching_prefetch_config.is_empty());


nit: Could this be a method on self.0 or self.0.trie_config?

let prefetch_enabled = !is_view && self.0.trie_config.prefetch_enabled();

wacban · 2024-04-02T07:42:02Z

core/store/src/config.rs

@@ -59,6 +59,9 @@ pub struct StoreConfig {
    /// This config option is temporary and will be removed once flat storage is implemented.
    pub sweat_prefetch_senders: Vec<String>,

+    pub claim_sweat_prefetch_config: Vec<PrefetchConfig>,
+    pub kaiching_prefetch_config: Vec<PrefetchConfig>,
+


nit: It would be nice to put all of the prefetch configs into a dedicated struct e.g. PrefetchConfig. The only concern is that if any nodes have this config overrided they may not notice and lose their configuration. Do you know if that is the case? Would it be sufficient to announce this change in the CHANGELOG?

It would make the code nicer but is a bit annoying so up to you if you want to do it.

wacban · 2024-04-02T07:42:15Z

core/store/src/config.rs

+
+#[derive(Clone, Debug, Default, serde::Serialize, serde::Deserialize)]
+#[serde(default)]
+pub struct PrefetchConfig {


Can you add a comment please?

wacban · 2024-04-02T07:43:23Z

runtime/runtime/src/prefetch.rs

-                        for action in &action_receipt.actions {
-                            if let Action::FunctionCall(fn_call) = action {
+                    for action in &action_receipt.actions {
+                        if let Action::FunctionCall(fn_call) = action {


nit:

let Action::FunctionCall(fn_call) = action else { continue; }

wacban · 2024-04-02T07:45:28Z

runtime/runtime/src/prefetch.rs

-                            if let Action::FunctionCall(fn_call) = action {
+                    for action in &action_receipt.actions {
+                        if let Action::FunctionCall(fn_call) = action {
+                            if self.prefetch_api.sweat_prefetch_receivers.contains(&account_id)


nit: Each of those ifs can be moved to a small helper method for cleaner code.

wacban · 2024-04-02T07:46:36Z

runtime/runtime/src/prefetch.rs

        if let Ok(json) = serde_json::de::from_slice::<serde_json::Value>(arg) {
-            if json.is_object() {
-                if let Some(list) = json.get("steps_batch") {
-                    if let Some(list) = list.as_array() {
-                        for tuple in list.iter() {
-                            if let Some(tuple) = tuple.as_array() {
-                                if let Some(user_account) = tuple.first().and_then(|a| a.as_str()) {
-                                    let hashed_account =
-                                        sha2::Sha256::digest(user_account.as_bytes()).into_iter();
-                                    let mut key = vec![0x74, 0x00];
-                                    key.extend(hashed_account);
-                                    let trie_key = TrieKey::ContractData {
-                                        account_id: account_id.clone(),
-                                        key: key.to_vec(),
-                                    };
-                                    near_o11y::io_trace!(count: "prefetch");
-                                    self.prefetch_trie_key(trie_key)?;
+            if let Some(list) = get_inner_json(&json, "steps_batch") {


nit: ditto on using the let ... else pattern for early returns to flatten the code

same in other methods below

wacban · 2024-04-02T07:49:39Z

runtime/runtime/src/prefetch.rs

+        Ok(())
+    }
+
+    fn prefetch_claim_sweat(&self, account_id: AccountId, arg: &[u8]) -> Result<(), PrefetchError> {


nit: Add a comment please. There are plenty of magic values and constants that were reverse engineered from the contract right? Let's make it easier for the next person who needs to adjust it after the contract is updated.

same for the other new methods.

and since you are already refactoring the initial sweat prefetcher, maybe also add a comment about that :)

You could add that let mut key = vec![0x74, 0x00]; is just a "t" string used as the unique prefix of the data structure terminated by a null value.

wacban · 2024-04-02T09:14:36Z

Also do we have any good metrics and logs for prefetcher? If not can you add those and prepare a dashboard?

mooori

I think I can review prefetch_kaiching. Otherwise, unfortunately I'm not familiar with the prefetcher implementation either. @Longarithm do you know someone who is?

+1 to Wac's comments, I think addressing them would make it much easier to follow/review this code. Especially the deep nestings are hard to follow.

This Zulip topic should probably be brought up as reference for the Kaiching prefetching.

Are there any prefetcher tests that should be updated or added?

mooori · 2024-04-02T10:57:33Z

runtime/runtime/src/prefetch.rs

+        Ok(())
+    }
+
+    fn prefetch_kaiching(&self, account_id: AccountId, arg: &[u8]) -> Result<(), PrefetchError> {


Comments with more details on which parts of the ft_transfer_on logic this prefetch covers would be very helpful, since there are multiple interactions with storage.

Another storage read is triggered by calling self.assert_deposit_whitelist(...) inside ft_transfer_on. Would trying to prefetch that too be worth it?

mooori · 2024-04-02T11:26:12Z

runtime/runtime/src/prefetch.rs

+                                    let reward_id = tuple.get(2).and_then(|a| a.as_str());
+                                    if user_account_serialize_result.is_some() {
+                                        if let Some(reward_id) = reward_id {
+                                            let user_account_key_hash = hash(&user_account_key);


CryptoHash is opaque, so if sha256 could be used (like in Sweat prefetching), it would be more clear what happens here.

Edit: In case the hashing algorithm of CryptoHash is an implementation detail this might even break when the hashing algorithm is changed. Though such change is probably unlikely.

jakmeier

The code looks like it could work. But it's impossible to say without comparing it to the smart contract code and deep analysis. I appreciate that a lot of work went into this. It's unlikely that reviewers will find the time to replicate the full analysis. So ultimately, we need to rely on testing how it performs.

I assume you did the testing. But I didn't look at any resting results, so I cannot say if those are showing without doubt that all prefetchers work as expected. And since the prefetches are all counted the same in this code, I imagine it could be easy to miss if one of the trie keys is not constructed properly. So if not done already, I would suggest to test each prefetcher separately and report how many prefetches it triggers and how by much it reduces the unprefetched trie node reads.

But I'll approve it already because I think even a wrong trie key wouldn't do much harm, it just wouldn't give the desired results.

And of course, it wouldn't hurt to address the comments raised regarding style and documentation (comments) by me and other reviewers before merging. 😄

jakmeier · 2024-04-03T07:22:12Z

runtime/runtime/src/prefetch.rs

+fn get_inner_json<'a>(json: &'a serde_json::Value, key: &str) -> Option<&'a serde_json::Value> {
+    if json.is_object() {
+        json.get(key)
+    } else {
+        None
+    }
+}
+


What is the difference between get_inner_json(value, "key") to just value.get("key")?
It seems the signature is pretty much the same as
https://docs.rs/serde_json/latest/serde_json/enum.Value.html#method.get

I think the only reason why the old code checked for is_object first was to make the code as obvious as possible, rather than nice and clean. That was a clear priority, given how quick it was all finalized. And maybe another reason was me naively assuming that the more this ugly code sticks out, the more likely we will remove it sooner rather than later...

Today, we can write cleaner code and review it properly, so let's remove unnecessary checks.

jakmeier · 2024-04-03T07:38:10Z

runtime/runtime/src/prefetch.rs

+        Ok(())
+    }
+
+    fn prefetch_claim_sweat(&self, account_id: AccountId, arg: &[u8]) -> Result<(), PrefetchError> {


and since you are already refactoring the initial sweat prefetcher, maybe also add a comment about that :)

You could add that let mut key = vec![0x74, 0x00]; is just a "t" string used as the unique prefix of the data structure terminated by a null value.

jakmeier · 2024-04-03T07:46:41Z

runtime/runtime/src/prefetch.rs

+                    for tuple in list.iter() {
+                        if let Some(tuple) = tuple.as_array() {
+                            if let Some(user_account) = tuple.first().and_then(|a| a.as_str()) {
+                                let mut key = vec![0, 64, 0, 0, 0];


That's an odd key to prefix... Why 5 bytes? Is this also a data structure prefix in the smart contract?
Looks like maybe a 0 as u8 followed by a 64 as u64 in little endian. Or is it maybe 16384 as u64 in little endian, followed by a separating null value?

jakmeier · 2024-04-03T07:50:23Z

runtime/runtime/src/prefetch.rs

+                                    let mut user_account_key = Vec::new();
+                                    user_account_key.extend([1, 109]);


Suggested change

let mut user_account_key = Vec::new();

user_account_key.extend([1, 109]);

let mut user_account_key = vec![1, 109];

Also, it would be interesting again to know what this key is. If we don't understand this 100%, it's hard to argue if it's safe to hardcode the value.

jakmeier · 2024-04-03T07:59:58Z

runtime/runtime/src/prefetch.rs

+                                            near_o11y::io_trace!(count: "prefetch");
+                                            self.prefetch_trie_key(trie_key)?;
+
+                                            let mut reward_key = vec![0, 24, 0, 0, 0];


Again, an explanation for the key seems quite important to have in comments. Let's not make the bad practice I introduced at the time the new norm. ;)

bowenwang1996 · 2024-04-03T23:54:46Z

@Longarithm could we merge this PR :)

nagisa · 2024-04-05T13:24:39Z

FWIW we can look and keep an eye on the prefetching graphs to get some intuition as to whether the prefetcher is working as we intend it to. But also, we should merge this sooner rather than later? We've a release pending soon and if my memory serves me right we wanted this PR in it?

cc @VanBarbascu

Longarithm · 2024-04-05T23:16:50Z

Addressed most comments, prioritising getting this into next release. Had some trouble with merging with prefetching logic on master but came through it.

Co-authored-by: Longarithm <the.aleksandr.logunov@gmail.com>

Longarithm requested a review from a team as a code owner March 28, 2024 20:25

Longarithm requested review from wacban, mooori and jakmeier March 28, 2024 20:25

prefetch v2

4d7396b

Longarithm force-pushed the prefetcher-v2 branch from a7c887d to 4d7396b Compare March 29, 2024 13:03

wacban reviewed Apr 2, 2024

View reviewed changes

mooori reviewed Apr 2, 2024

View reviewed changes

jakmeier approved these changes Apr 3, 2024

View reviewed changes

Longarithm added 4 commits April 6, 2024 02:59

address most of comments

ff245ff

Merge branch 'master' into prefetcher-v2

242f902

fix failed merge

a2e7061

fix failed merge 2

964bb71

Longarithm enabled auto-merge April 5, 2024 23:15

Longarithm added this pull request to the merge queue Apr 5, 2024

Merged via the queue into near:master with commit 78f0c31 Apr 5, 2024
27 of 29 checks passed

Longarithm deleted the prefetcher-v2 branch April 5, 2024 23:52

Longarithm added a commit to Longarithm/nearcore that referenced this pull request Apr 6, 2024

feat: prefetch more trie keys (near#10899)

4f35b36

Co-authored-by: Longarithm <the.aleksandr.logunov@gmail.com>

VanBarbascu pushed a commit that referenced this pull request Apr 8, 2024

[1.39] prefetch more trie keys (#10899) (#10968)

9cc11e7

Co-authored-by: Longarithm <the.aleksandr.logunov@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: prefetch more trie keys #10899

feat: prefetch more trie keys #10899

Longarithm commented Mar 28, 2024

mooori commented Mar 29, 2024

Longarithm commented Mar 29, 2024 •

edited

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

wacban left a comment

wacban Apr 2, 2024

wacban Apr 2, 2024

wacban Apr 2, 2024

wacban Apr 2, 2024

wacban Apr 2, 2024

wacban Apr 2, 2024

wacban Apr 2, 2024

wacban Apr 2, 2024

jakmeier Apr 3, 2024

wacban commented Apr 2, 2024

mooori left a comment

mooori Apr 2, 2024

mooori Apr 2, 2024 •

edited

Loading

jakmeier left a comment

jakmeier Apr 3, 2024

jakmeier Apr 3, 2024

jakmeier Apr 3, 2024

jakmeier Apr 3, 2024

jakmeier Apr 3, 2024

jakmeier Apr 3, 2024

bowenwang1996 commented Apr 3, 2024

nagisa commented Apr 5, 2024 •

edited

Loading

Longarithm commented Apr 5, 2024

		let mut user_account_key = Vec::new();
		user_account_key.extend([1, 109]);

	let mut user_account_key = Vec::new();
	user_account_key.extend([1, 109]);
	let mut user_account_key = vec![1, 109];

feat: prefetch more trie keys #10899

feat: prefetch more trie keys #10899

Conversation

Longarithm commented Mar 28, 2024

mooori commented Mar 29, 2024

Longarithm commented Mar 29, 2024 • edited Loading

codecov bot commented Mar 29, 2024 • edited Loading

Codecov Report

wacban 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

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

wacban commented Apr 2, 2024

mooori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mooori Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

jakmeier 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowenwang1996 commented Apr 3, 2024

nagisa commented Apr 5, 2024 • edited Loading

Longarithm commented Apr 5, 2024

Longarithm commented Mar 29, 2024 •

edited

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

mooori Apr 2, 2024 •

edited

Loading

nagisa commented Apr 5, 2024 •

edited

Loading