runtime: prefetch account keys and data for chunk finalization

[nagisa]

This significantly reduces the cost of finalize, especially for shard 3 where they were the largest:

The effect on the overall apply duration is not as high however:

This is understandable, as ultimately all of this data that we were seeing in finalize still needs to be retrieved. The important part, however is that the duration of this is much more consistent and less jumpy now.

cc @saketh-are keep an eye out on whether we need to prefetch receipt data for yield-resume receipts.

[nagisa]

The benefit to the mean apply times is actually much more visible for shards 2 and 4. Shard 2 for example:

But you can also see that higher percentiles don't benefit as much and we still have a pretty long tail.

[codecov]

Codecov Report

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

Project coverage is 71.52%. Comparing base (f37edc0) to head (390d2be).
Report is 4 commits behind head on master.

Files	Patch %	Lines
runtime/runtime/src/prefetch.rs	89.47%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #10935   +/-   ##
=======================================
  Coverage   71.52%   71.52%           
=======================================
  Files         758      758           
  Lines      151686   151704   +18     
  Branches   151686   151704   +18     
=======================================
+ Hits       108489   108506   +17     
- Misses      38708    38712    +4     
+ Partials     4489     4486    -3     

Flag	Coverage Δ
backward-compatibility	0.24% <0.00%> (-0.01%)	⬇️
db-migration	0.24% <0.00%> (-0.01%)	⬇️
genesis-check	1.43% <0.00%> (+<0.01%)	⬆️
integration-tests	37.03% <15.55%> (-0.16%)	⬇️
linux	70.01% <91.11%> (+0.01%)	⬆️
linux-nightly	71.01% <91.11%> (-0.02%)	⬇️
macos	54.49% <53.33%> (+0.01%)	⬆️
pytests	1.66% <0.00%> (+<0.01%)	⬆️
sanity-checks	1.44% <0.00%> (+<0.01%)	⬆️
unittests	67.17% <88.88%> (-0.01%)	⬇️
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.

[jakmeier]

Looks good, can't argue with the data! :D

I think the observed problem came in when we added flat storage, long after the prefetcher itself. Before that, anything read in finalize() was already read before. But now that flat storage is "cheating", we have to pay the full price in finalize().

Putting that work on the prefetcher makes sense. But it's stretching it, as it was designed to reduce latency on receipt processing. The idea of prefetching for something further ahead than a 10-100ms was not really intended. Hence you were forced to this somewhat hacky approach, I guess.

Never the less, awesome that it works so well!

[jakmeier]

Just to point out the obvious, confirming what you already wrote:
Removing all those clear() calls means we do more overfetching and we have less efficient usage of the prefetcher's resources (IO bandwith, threads, staging area, request queue). This will generally lead to more useless data being fetched and higher chances that we are too late to fetch the data we actually need.

One of the concerns is that we don't know how many receipts we even need to execute from the delayed queue. But we prefetch for all of them. So, if you have 100 delayed sweat batch-updates in the queue, you will be fetching all those, even if only 4 fit in a chunk.

Even worse, although usually not happening, if a sweat batch receipt fail for some reason, we might be left with a full staging area and cannot prefetch anything until we clear it.

But yeah, in the end I agree it's better to not clear as things stand today, since we need this data in finalize.

[saketh-are]

IIUC we check compute limit as we go through the delayed receipts and the prefetching happens inside the loop:

nearcore/runtime/runtime/src/lib.rs

Lines 1487 to 1507 in 1d082db

	// Then we process the delayed receipts. It's a backlog of receipts from the past blocks.
	while delayed_receipts_indices.first_index < delayed_receipts_indices.next_available_index {
	if total.compute >= compute_limit
	|| proof_size_limit
	.is_some_and(|limit| state_update.trie.recorded_storage_size() > limit)
	{
	break;
	}
	let key = TrieKey::DelayedReceipt { index: delayed_receipts_indices.first_index };
	let receipt: Receipt = get(&state_update, &key)?.ok_or_else(|| {
	StorageError::StorageInconsistentState(format!(
	"Delayed receipt #{} should be in the state",
	delayed_receipts_indices.first_index
	))
	})?;
	if let Some(prefetcher) = &mut prefetcher {
	prefetcher.clear();
	// Prefetcher is allowed to fail
	_ = prefetcher.prefetch_receipts_data(std::slice::from_ref(&receipt));
	}

[saketh-are]

The downside of that is there is very little delay between when the prefetcher is invoked for the receipt and when the receipt is actually processed.

Could it be worth trying:

1. Implement a new prefetcher method which chains prefetch_trie_key into prefetch_receipts_data. We could call it with a trie key DelayedReceipt { index } and basically it would understand that the trie value is expected to be a receipt and it should perform prefetching for the receipt as well.
2. Call the new prefetcher method on the next index in the delayed receipts queue while we are processing the current one. Or in general, have some sliding window of K indices over which we invoke the prefetcher.

[nagisa]

I don't think we need to do anything as fancy. We can take a guess that we fit e.g. "about 500 delayed receipts" and start prefetching them even before we start executing transactions at the beginning of the apply. Even if we are overzealous with how many we do end up prefetching for that specific chunk, its not like those receipts are going anywhere -- we'll have to process them next iteration around anyway. And at that point the receipts would still be in some of the caches, so they would be faster to prefetch on the next iteration.

It is true that we prefetch them just before they are necessary currently, and at least some of the metrics do indicate that before the clear() call was moved to-after-finalize we would "cancel" some prefetch requests quite consistently (suggesting that we indeed prefetch a little too Just In Time.)

Regardless these are all good thoughts to evaluate if and when we improve on this code. I don't think we strictly need to add to this PR to address anything here so far.

[jakmeier]

This sounds useful, indeed.
Then it would also make sense to aggressively clear the "required soon" queue but basically never touch the other queue.

[jakmeier]

Actually, if we follow the initial design of the prefetcher, this means we are just sending those prefetch requests too early. The prefetches are made before processing but we need the values after processing.

So, what I would have tried first is this: Instead of prefetching the access keys and account data in the prefetch_receipt_data() call, I would have added a new method to the prefetcher. Maybe something like prefetch_finalize where we specifically read data that was written but never read. Sure, we start it a bit later, but it's still with 8 background threads + 1 main thread instead of just the main thread.
Then I could still do the clearing in all places but guarantee parallel fetches on the data for finalize.

But what you have seems to work good enough, and I'm not sure my approach would actually give equal or better results.

[nagisa]

I'm happy to explore this idea… as a follow up. Thank you for writing it down :)