feat: prefetch accounts and access keys #7590

jakmeier · 2022-09-08T22:43:54Z

Introduces the concept of prefetching data from the DB while applying chunks.

This is non-speculative prefetching only for now. In other words, the future is not speculatively predicted, only data is fetched that is guaranteed to be useful. More details on that inside runtime/runtime/src/prefetch.rs.

No protocol change is needed for this. In general, prefetching how it has been implemented is (supposed to be) invisible in all possible ways, other than trie storage latency.

Performance wise, this is going to make the worst-case assumption for all action receipts better. The worst case is that two accounts and one access keys have to be fetched from disk for every receipt. This IO cost dominates the gas cost for action receipt creation.

Prefetching this data opens the door to potentially reducing this cost. This could affect all actions but is particularly relevant for redistributing gas costs around function call actions, see also #6992.

Test plan

Tests check that the prefetcher loads the trie nodes that are expected into the staging area and that they are removed from it afterwards.

Standard rocksdb `flush()` only flushes the default column family. See https://github.com/facebook/rocksdb/blob/95ef007adc9365fbefc0f957722a191c1fd7dcd3/include/rocksdb/db.h#L1398-L1400 To flush all column families as intended, iterator over them and flush them individually.

Longarithm · 2022-09-09T13:55:17Z

core/store/src/trie/prefetching_trie_storage.rs

+    /// Work items are defined as `TrieKey` because currently the only
+    /// work is to prefetch a trie key. If other IO work is added, consider
+    /// changing the queue to an enum.
+    work_queue: Arc<Mutex<BoundedQueue<TrieKey>>>,


If I got it right, BoundedQueue is not useful here - if it is full, it pops and returns element from tail, instead of returning new element back. We may need different queue implementation here, sorry if naming was misleading

Oh, I see, my mistake. I switched to the bounded queue last minute, doing manual checks over a VecDequeue should be simple enough anyway, I don't think I need a new queue type.

probably I should add tests for the memory bounds too, this one should be caught in a test IMO

Ekleog

There are quite a few things I did not understand, so I commented on them. Also maybe a few ideas for consideration in the inline comments? Though given the things I don't understand yet I'm not sure all of them would be good ones.

Also, this review is focused only on the concurrency parts of it, as the storage parts of things is beyond what I'm used to.

core/store/src/trie/prefetching_trie_storage.rs

Ekleog · 2022-09-09T12:52:43Z

core/store/src/trie/prefetching_trie_storage.rs

+    /// pre-fetcher uses this in read-only mode to avoid premature evictions.
+    shard_cache: TrieCache,
+    /// Shared with parent `TrieCachingStorage`.
+    prefetching: Arc<Mutex<PrefetchStagingArea>>,


If I understand correctly, the reads and writes that can happen here are:

Adding a slot when submitting a job. This mutates the hashmap itself

Filling a slot when a job completes. This mutates only one value of the hashmap

Reading a slot when a job is needed. This reads only one value of the hashmap

Releasing a slot after the job is needed. This writes the hashmap

With this in mind, to ideally encode this in the rust type system, I would:

Hoist the Mutex inside PrefetchStagingArea so that it's Sync and users of the type don't need to care about synchronization (AFAICT we never use nor will use PrefetchStagingArea outside of a Mutex)

Hoist the slots for step 2 and 3 outside of the HashMap: now, the HashMap is Hash → Id with an invariant that Id is unique in the HashMap

Store the Slots outside of the HashMap, in a Vec<Mutex> indexed by the Id above. So this Vec would be stored outside of the HashMap mutex described in step 1. Hopefully we don't need to change the Vec size while running, if we did need to we could use a RwLock around the Vec because it should basically never happen

Now, steps 2 and 3 would probably be increasing code complexity for little benefit so long as there's little contention, and my guess would be there would be little contention. So maybe just consider step 1 for now, as it cleans up the API, and keep steps 2 and 3 for if benchmarks say it'd be a good idea?

I really like your suggestions here! I will probably only incorporate 1. for now, to make the code as simple as possible.

Applies step one in my latest commit

Ekleog · 2022-09-09T13:07:35Z

core/store/src/trie/prefetching_trie_storage.rs

+    // The shard cache mutex plus the prefetch staging area mutex are used for
+    // that in combination. Let's call the first lock S and the second P.
+    // The rules for S and P are:
+    // 1. To avoid deadlocks, S must always be requested before P, if they are


Step 1 would also solve this in a typesystem-based way, so this comment would become unnecessary

I'm not sure how step 1 helps here. I think I applied it roughly like you suggested. But the shard cache still lives next to the staging area in TriePrefetchingStorage and in TrieCachingStorage. Both could still lock in the wrong order from a type-system point of view.
Did I misunderstand step 1, or am I missing something else? It would be lovely if this could be incorporated in the type system!

My thinking was that if the mutex is inside PrefetchStagingArea, then it is the only thing that will ever lock it, and by scoping it cannot hold the lock for long enough to cause a deadlock. That said I haven't re-reviewed the PR yet so I may come back in a bit with additional details

core/store/src/trie/prefetching_trie_storage.rs

Ekleog · 2022-09-09T14:34:36Z

runtime/runtime/src/lib.rs

@@ -1319,6 +1335,10 @@ impl Runtime {
        }

        // And then we process the new incoming receipts. These are receipts from other shards.
+        if let Some(prefetcher) = &mut prefetcher {
+            prefetcher.clear();
+            let _queue_full = prefetcher.input_receipts(&incoming_receipts);


And same here, this could be moved alongside the two other input_receipts I think

Indeed it could and I had it this way in my first implementation.
I think it makes more sense here because we want to fetch this data after all delayed receipts have finished, if they even finish all in this block. Also, the bounded queue of requests will have more space again even if the local receipts already filled it up.
That was my thinking. But I am open to arguments to move it up if you still think it makes more sense there.

Ekleog · 2022-09-09T14:37:01Z

runtime/runtime/src/lib.rs

@@ -1165,6 +1167,11 @@ impl Runtime {

        let trie = Rc::new(trie);
        let mut state_update = TrieUpdate::new(trie.clone());
+        let mut prefetcher = TriePrefetcher::new(trie.clone());


I don't understand, why do you recreate a prefetcher from scratch, including spinning up new threads, for each new block? Would it not be possible to just reuse the same thread pool and metadata for all the blocks, just clearing out the slots to make space for the new prefetch requests?

Hm, it would be nicer to keep the prefetcher and threads alive, yeah. The trie root of each IO thread's PrefetchingTrieStorage would have to be updated, as each chunk operates on a different trie. It can also be tricky because chunks are already executed by a rayon iterator, so we don't even know exactly how many chunks are being processed at a time.
And we would need to keep state between chunks somewhere and pass it down. In other words, the change would spill much further up in the transaction runtime, while I was trying to keep it local.

How bad do you think it is to create a new set of threads each time? If it is only a problem due to performance concerns, it might be okay to current design, my tests showed that creating the threads only costs single digit micro seconds.

Ekleog · 2022-09-09T14:37:35Z

runtime/runtime/src/lib.rs

@@ -1331,6 +1351,11 @@ impl Runtime {
            }
        }

+        // No more receipts are executed on this trie, stop any pending prefetches on it.
+        if let Some(prefetcher) = &prefetcher {
+            prefetcher.stop_prefetching();


This should maybe be part of the impl Drop for TriePrefetcher

That's already what the current impl Drop for TriePrefetcher does effectively. It is required anyway for all failure cases that may happen above.

Would you like to see drop(prefetcher) here instead of calling stop_preftching?

Hmm I think it would be dropped by end-of-scope anyway? (unless the code changes, I'l re-mention it in the new if it's important anyway)

runtime/runtime/src/prefetch.rs

Ekleog · 2022-09-09T14:44:38Z

runtime/runtime/src/prefetch.rs

+
+    /// Start prefetching data for processing the receipts.
+    ///
+    /// Returns an error if the prefetching queue is full.


Thinking about it now, AFAICT the queue is already semantically bounded by the number of incoming transactions and receipts, which are all already held in memory, and gets cleared at the end of each block.

So I'm starting to wonder whether the queue should not just be unbounded, as sure it'll increase the memory use constant a bit, but this way it avoids the situation where data that could have been prefetched was not due to the queue being bounded?

With the follow-up PR that is in the works, the amount of requests per receipts will no longer be bounded by a reasonable constant. So, while in the context of only prefetching accounts and access keys your argument makes a lot of sense, the more general prefetching I want to introduce will potentially read many items per receipts.
To give a concrete example, imagine 10 receipts are ready. Each of them is a IO heavy function call that we can somehow prefetch. Each will end up using 299 Tgas, so in reality we will only process 4 of them. Fetching data for all 10 receipts would be too much in this case.
Using a bounded queue is a way to prevent that we will prefetch more data than we can feasibly fetch within one block, without the need to know or predict how many of the receipts will be executed this block.

Would it actually be bad to prefetch too much? Actually, thinking about it: when switching to a new trie root, should we not dump all the prefetched and unused-yet data into shard cache, as it'll most likely be used for the next block anyway? (Though we shouldn't do that if it'd affect gas pricing, but I don't know about that)

Co-authored-by: Léo Gaspard <github@leo.gaspard.ninja>

- use crossbeam `ArrayQueue` - this also fixes wrong usage of `BoundedQueue` - put arc and mutex inside `PrefetchStagingArea` - fixed false comments

Longarithm · 2022-09-12T19:04:53Z

core/store/src/trie/prefetching_trie_storage.rs

+const MAX_PREFETCH_STAGING_MEMORY: usize = 200 * 1024 * 1024;
+/// How much memory capacity is reserved for each prefetch request.
+/// Set to 4MiB, the same as `max_length_storage_value`.
+const PREFETCH_RESERVED_BYTES_PER_SLOT: usize = 4 * 1024 * 1024;


It means that if 50 slots are occupied, we can't insert new prefetch requests anymore. One receipt can include a batch of 100 storage reads, which itself may trigger, say 20 nodes read, so such scenario would require 2K slots.
Is prefetcher fast enough to keep the queue not full? If not, can we call Trie::get_ref instead of Trie::get - so we could reduce value size limit to 1K - node size limit?

I should clarify this in the comment: This is the reserved bytes before we start fetching, at which point we assume the worst case. But once we have the value, we will use the actual size. So with 8 IO threads, we only charge this pessimistic value for 8 slots at a time. With that in mind, the prefetcher is fast enough.
If we wanted to go for many more threads, I think your idea would be perfect to address this issue. But as things stand, I tend towards prefetching everything.

- by default have prefetching disabled, allow enabling it with config - keep prefetcher and IO threads alive between chunks - use crossbeam bounded channel - remove stop_io atomic boolean - change some comments

jakmeier · 2022-09-13T00:48:59Z

@Longarithm @mm-near @Ekleog
I pushed a few more changes.

Now the feature has to be enabled in the config, otherwise it's dead code.
Using crossbeam channels removed some ugly sleeps, as recv blocks until a message arrives or the channel is disconnected.
I moved the ownership of PrefetchApi to be alongside the shard caches in ShardTries. This means they are kept alive between chunks of the same shard, and shared in case of multiple chunks of the same shard being applied simultaneously. A clone of the api object is then placed inside TrieCachingStorage, which the node runtime can pick up to send requests.
To make simultaneous chunk processing work, I include the trie root in every request. Not pretty but by far the simplest solution that I could come up with.
As a nice side-effect, the atomic boolean to manually kill threads is no longer needed.

I think I broke some tests with the config, need to check that out tomorrow... My own tests are running fine though, so prefetching still works as intended, if enabled in config.

core/store/src/trie/prefetching_trie_storage.rs

mm-near · 2022-09-13T06:47:48Z

core/store/src/trie/prefetching_trie_storage.rs

+#[derive(Clone, Debug)]
+enum PrefetchSlot {
+    PendingPrefetch,
+    PendingFetch,


What happens if Prefetching failed ? What do we put in the slot ?

Prefetching is always done by hash, and hashes must be present in node storage. So if there is a failure, it is an actual IO error or a missing node, neither of which we can handle anyway.
I've now changed it that we will at least remove the reserved slot in case of a failure, but I don't see any value in putting the error value in there. Does that make sense?

yeah - removing the reserved slot makes sense. thanks. -- this will be especially important for sweatcoin and other future optimizers -- that might in theory be requesting things that don't exist in storage.

core/store/src/trie/prefetching_trie_storage.rs

mm-near · 2022-09-13T07:14:31Z

core/store/src/trie/prefetching_trie_storage.rs

+                    .map_err(|_| StorageError::StorageInternalError)?
+                    .ok_or_else(|| {
+                        StorageError::StorageInconsistentState("Trie node missing".to_string())
+                    })?


question about error handling:

so if we have an error here -- the self.prefetching.slots will not be updated - so it will keep having PendingPrefetch there -- and blocking_get will block forever ?

You are right, this isn't ideal. Error are unrecoverable at this point but we don't want to end up in a infinite loop anyway.
The fix I just pushed handles None and Err separately and has some comments. And we release the slot such that any thread waiting on the value can progress. Additionally, the main thread will try again fetching the data on its own, just in case something is fishy with the store in the prefetcher.

core/store/src/trie/prefetching_trie_storage.rs

core/store/src/trie/trie_storage.rs

mm-near · 2022-09-13T07:26:21Z

core/store/src/trie/trie_storage.rs


                // Insert value to shard cache, if its size is small enough.
                // It is fine to have a size limit for shard cache and **not** have a limit for chunk cache, because key
                // is always a value hash, so for each key there could be only one value, and it is impossible to have
                // **different** values for the given key in shard and chunk caches.
                if val.len() < TrieConfig::max_cached_value_size() {
+                    let mut guard = self.shard_cache.0.lock().expect(POISONED_LOCK_ERR);


so in case of the "old" behaviour -- we acquire the lock again (while holding it already) - does it lead to deadlocks ?

Right, good catch. I think we should just drop the guard in any case above. The only parallel access you could have to the shard cache in the absence of prefetchers would be when multiple chunks for the same shard are applied at the same time. And I see no reason that we need to keep the lock in between, in fact releasing it makes that scenario much more efficient.

- comments - avoid double lock of same mutex - handle prefetcher errors more gracefully

Longarithm · 2022-09-13T12:04:33Z

I don't have prior experience with prefetching logic. I spent several hours reading, the implementation looks clean and reasonable. From what I see, now we have a memory shared with caching storage and prefetchers, from which we take fetched values - which make strong sense.

I leave an approval, with one more comment I came up recently.

core/store/src/trie/trie_storage.rs

mm-near · 2022-09-13T13:56:27Z

core/store/src/trie/trie_storage.rs

+                    std::mem::drop(guard);
+
+                    val = match prefetch_state {
+                        // Slot reserved for us, or no space left. Main thread should fetch data from DB.


we are the "main" thread, right ?

especially the "Slot reserved" case deserve a better comment -- AFAIK this means, that we asked the prefetcher, but it didn't pick it up yet -- so we're doing it outselves..

added some more comments, let me know @mm-near if you think it is still unclear

mm-near · 2022-09-13T13:59:12Z

core/store/src/trie/prefetching_trie_storage.rs

+#[derive(Clone, Debug)]
+enum PrefetchSlot {
+    PendingPrefetch,
+    PendingFetch,


yeah - removing the reserved slot makes sense. thanks. -- this will be especially important for sweatcoin and other future optimizers -- that might in theory be requesting things that don't exist in storage.

jakmeier · 2022-09-13T14:13:11Z

I don't have prior experience with prefetching logic. I spent several hours reading, the implementation looks clean and reasonable. From what I see, now we have a memory shared with caching storage and prefetchers, from which we take fetched values - which make strong sense.

I leave an approval, with one more comment I came up recently.

That's great, thanks for the review! It is a complex PR on several dimensions. I really appreciate it that you put in the time as someone who understand the trie storage details better than I do. That's where we really needed your expertise a reviewer the most.

For prefetching details, I was able to talk it through with several people in person and I think the design makes sense in principle.

Ekleog

Requesting changes for the "leaking keys in prefetch staging area" issue, that'd mean prefetching would stop working after enough blocks happen until the process is restarted.

Overall LGTM otherwise, though I added comments and am wondering about gas costs' relationship with shard cache

Ekleog · 2022-09-13T14:40:15Z

core/store/src/trie/prefetching_trie_storage.rs

+                        // `blocking_get` will return None if the prefetch slot has been removed
+                        // by the main thread and the value inserted into the shard cache.
+                        let mut guard = self.shard_cache.0.lock().expect(POISONED_LOCK_ERR);
+                        guard.get(hash)


Is this useful? I'm under the impression that the return value of TriePrefetchingStorage-backed tries is just ignored. So if the value was inserted in the shard cache anyway, I think we can just return Arc::new([])?

(And maybe do that literally in every return of TriePrefetchingStorage, as it could lead to surprising behavior otherwise)

only the final return value is ignored, all the node that are fetched through the same interface are required to traverse the trie

so if I understand the question right, then yes, it is very useful :)

Ekleog · 2022-09-13T14:42:51Z

core/store/src/trie/prefetching_trie_storage.rs

+                    })
+                    .ok_or_else(|| {
+                        // This could only happen if this thread started prefetching a value
+                        // while also another thread was already prefetching it. Then the other


Same here, I think the locking on the prefetching slots hashmap is enough to prevent this situation from ever happening, and it could semantically be a panic? Not that it's not good to handle it anyway, but I'm thinking the comment and error message could be more explicit

I don't understand, as far as I see we are not holding the lock for the prefetching area continuously.

core/store/src/trie/prefetching_trie_storage.rs

Ekleog · 2022-09-13T14:54:56Z

core/store/src/trie/prefetching_trie_storage.rs

+    /// Queued up work will not be finished. But trie keys that are already
+    /// being fetched will finish.
+    pub fn clear(&self) {
+        while let Ok(_dropped) = self.work_queue_rx.try_recv() {}


I think this could cause spurious prefetch failures if two chunks are being processed in parallel nad one finishes while the other is still prefetching. That said it's probably something we can postpone the fix for later, so long as an issue is kept to track this lack

there are such issues, I tried to cover them in new comments and also explain how we deal with them currently

core/store/src/trie/shard_tries.rs

Ekleog · 2022-09-13T15:06:46Z

core/store/src/trie/trie_storage.rs

                    guard.put(*hash, val.clone());
                } else {
                    self.metrics.shard_cache_too_large.inc();
                    near_o11y::io_trace!(count: "shard_cache_too_large");
                }

+                if let Some(prefetcher) = &self.prefetch_api {
+                    // Only release after insertion in shard cache. See comment on fn release.
+                    prefetcher.prefetching.release(hash);


If one of the read_from_db calls above early-return, or if future code changes lead to other early-return points being added in this function, we'll leak one slot. Not sure how bad it is though, as the prefetch area should be emptied after each chunk, so maybe we can just live with this.

I guess yeah, but if we have a storage error on the main thread I am pretty sure we panic on the caller site anyway. Storage errors of that kind are unrecoverable AFAIK.
Anyway, with the clear between chunks, I believe this should also be handled indirectly now.

Ekleog · 2022-09-13T15:10:06Z

runtime/runtime/src/lib.rs

@@ -1331,6 +1351,11 @@ impl Runtime {
            }
        }

+        // No more receipts are executed on this trie, stop any pending prefetches on it.
+        if let Some(prefetcher) = &prefetcher {
+            prefetcher.clear();


We definitely need to remove from the prefetch staging area all the keys that were related to this trie root here, as otherwise all the keys that were prefetched and not consumed because processing had to stop would leak and take space forever, making prefetching less and less efficient

runtime/runtime/src/prefetch.rs

Ekleog

Overall LGTM! I just think we should open an issue to track improving the "drop the whole hashmap upon chunk end" behavior, as it's something that'll go worse the more our system is under load, so we should probably fix it before it becomes a problem we see in the real world as then it'd be too late.

Also for all my comments about size_guard handling, I'd feel even better if it were possible, instead, to do the refactor "size+hashmap -> SizeBoundHashMap" struct as we were discussing yesterday. It'd reduce the risk some of the size computations done here were wrong (or will become wrong with future changes), as I'm not sure I didn't miss any place size is being updated. (While I do think that the current code is correct, an underflow may have pretty bad consequences, so...)

Ekleog · 2022-09-14T09:45:06Z

core/store/src/trie/prefetching_trie_storage.rs

+    /// Get prefetched value if available and otherwise atomically set
+    /// prefetcher state to being fetched by main thread.
+    pub(crate) fn get_or_set_fetching(&self, key: CryptoHash) -> PrefetcherResult {
+        self.get_and_set_if_empty(key, PrefetchSlot::PendingFetch)


Nit: this function could probably just be removed and inlined to its (I think) only caller, as I don't think it clarifies code much

it's done to keep PrefetchSlot private to this module, which I feel does clarify code a lot in some sense

Oh yes I didn't think of visibility concerns :)

Ekleog · 2022-09-14T09:45:35Z

core/store/src/trie/prefetching_trie_storage.rs

+
+    fn insert_fetched(&self, key: CryptoHash, value: Arc<[u8]>) {
+        let mut guard = self.0.lock().expect(POISONED_LOCK_ERR);
+        guard.size_bytes += value.len();


Nit: use Self::reserved_memory here

Ekleog · 2022-09-14T09:47:14Z

core/store/src/trie/prefetching_trie_storage.rs

+    /// Reserved memory capacity for a value from the prefetching area.
+    fn reserved_memory(dropped: PrefetchSlot) -> usize {
+        match dropped {
+            PrefetchSlot::Done(value) => value.len(),


Maybe this should be value.len() + 16 (fat arc), or even also counting the key size, so that we avoid issues around spamming the prefetch cache with empty or near-empty values? (Though with the clean up and the fact we don't prefetch whatever the user wants yet it's probably not a big deal anyway, but just for future-proofing)

my personal feeling is that a boundary on value sizes is easier to reason about than actual memory usage, so I'd like to keep it as it is for now
I would change my opinion if the total size was really large - but this should stay relatively small anyway.

Ekleog · 2022-09-14T09:52:24Z

core/store/src/trie/prefetching_trie_storage.rs

+                    return PrefetcherResult::MemoryLimitReached;
+                }
+                entry.insert(set_if_empty);
+                guard.size_bytes += PREFETCH_RESERVED_BYTES_PER_SLOT;


Same here, this should be Self::reserved_memory in case we ever add a caller that inputs a Done directly to the cache

Ekleog · 2022-09-14T09:59:49Z

core/store/src/trie/prefetching_trie_storage.rs

+        for (_key, dropped) in guard.slots.drain() {
+            reclaimed += PrefetchStagingArea::reserved_memory(dropped);
+        }
+        guard.size_bytes -= reclaimed;


Why not just update the size in the loop? It's not even an atomic so performance-wise it should be the exact same.

Andeven more: why not just reset the size to 0 flat? It should end up at 0 anyway, so I can see the "reclaimed" variable being useful exclusively within a debug_assert.

in the loop is not possible due to double borrow, but I can see the argument to set it to 0 flat

Ekleog · 2022-09-14T10:10:47Z

core/store/src/trie/trie_storage.rs

+                        // It only means we were not able to mark it as already being fetched, which in turn could lead to
+                        // a prefetcher trying to fetch the same value before we can put it in the shard cache.
+                        PrefetcherResult::SlotReserved | PrefetcherResult::MemoryLimitReached => {
+                            self.read_from_db(hash)?


Hmm no prefetch_miss metric here, just for completeness?

actual metrics are added in the other PR, and the io trace metrics you see here are for post-processing where we can just subtract one number from another to get misses anyway

Ekleog

Looks great to me! :D

Introduces the concept of prefetching data from the DB while applying chunks. This is non-speculative prefetching only for now. In other words, the future is not speculatively predicted, only data is fetched that is guaranteed to be useful. More details on that inside `runtime/runtime/src/prefetch.rs`. No protocol change is needed for this. In general, prefetching how it has been implemented is (supposed to be) invisible in all possible ways, other than trie storage latency. Performance wise, this is going to make the worst-case assumption for all action receipts better. The worst case is that two accounts and one access keys have to be fetched from disk for every receipt. This IO cost dominates the gas cost for action receipt creation. Prefetching this data opens the door to potentially reducing this cost. This could affect all actions but is particularly relevant for redistributing gas costs around function call actions, see also near#6992. ---- Tests check that the prefetcher loads the trie nodes that are expected into the staging area and that they are removed from it afterwards.

Introduces the concept of prefetching data from the DB while applying chunks. This is non-speculative prefetching only for now. In other words, the future is not speculatively predicted, only data is fetched that is guaranteed to be useful. More details on that inside `runtime/runtime/src/prefetch.rs`. No protocol change is needed for this. In general, prefetching how it has been implemented is (supposed to be) invisible in all possible ways, other than trie storage latency. Performance wise, this is going to make the worst-case assumption for all action receipts better. The worst case is that two accounts and one access keys have to be fetched from disk for every receipt. This IO cost dominates the gas cost for action receipt creation. Prefetching this data opens the door to potentially reducing this cost. This could affect all actions but is particularly relevant for redistributing gas costs around function call actions, see also #6992. ---- ### Test plan Tests check that the prefetcher loads the trie nodes that are expected into the staging area and that they are removed from it afterwards.

jakmeier added 11 commits August 30, 2022 16:46

squashed prefetcher poc

11a9c4b

limit scheduler queue and share io request queue

8e0cd00

enforce memory limits

c7d31be

move prefetching storage in separate module

fb99226

wip

2ba42f9

Merge remote-tracking branch 'upstream/master' into prefetch-accounts

a5c1755

add tests

f7514e0

also prefetch delayed receipts

14bb8a2

simplify API and clean up for review

f4b53d5

move test-only function

344bb45

jakmeier requested a review from a team as a code owner September 8, 2022 22:43

jakmeier requested review from matklad and Longarithm and removed request for matklad September 8, 2022 22:43

Longarithm reviewed Sep 9, 2022

View reviewed changes

Ekleog reviewed Sep 9, 2022

View reviewed changes

jakmeier and others added 5 commits September 10, 2022 23:37

Update runtime/runtime/src/prefetch.rs

8aeee82

Co-authored-by: Léo Gaspard <github@leo.gaspard.ninja>

apply reviewer suggested refactoring

bf6d12b

- use crossbeam `ArrayQueue` - this also fixes wrong usage of `BoundedQueue` - put arc and mutex inside `PrefetchStagingArea` - fixed false comments

simplify indentation

d57e094

fix get_and_set_if_empty memory limit check order

931d566

add cargo.lock changes

d714200

Longarithm reviewed Sep 12, 2022

View reviewed changes

jakmeier added 3 commits September 13, 2022 02:04

more improvements from reviewer suggestions

07bd456

- by default have prefetching disabled, allow enabling it with config - keep prefetcher and IO threads alive between chunks - use crossbeam bounded channel - remove stop_io atomic boolean - change some comments

handle case of parallel chunk execution

de99c15

finish impl for keeping prefetcher alive

5107eda

mm-near reviewed Sep 13, 2022

View reviewed changes

jakmeier added 2 commits September 13, 2022 10:34

address marcin's review comments

09db2f2

- comments - avoid double lock of same mutex - handle prefetcher errors more gracefully

Don't treat non-existing keys as failure

de4f395

mm-near reviewed Sep 13, 2022

View reviewed changes

improve comments

3844b15

Ekleog suggested changes Sep 13, 2022

View reviewed changes

resolve capacity leakage issues

8cccc75

jakmeier requested review from Ekleog and mm-near and removed request for Ekleog September 14, 2022 06:35

mm-near approved these changes Sep 14, 2022

View reviewed changes

disable prefetching for view calls

a4731d3

jakmeier force-pushed the prefetch-accounts branch from ed0bcfd to a4731d3 Compare September 14, 2022 10:13

add comment

9a8935a

Ekleog reviewed Sep 14, 2022

View reviewed changes

refactor: size tracked hash map

261fa56

Ekleog approved these changes Sep 14, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into HEAD

e82f409

jakmeier added the S-automerge label Sep 14, 2022

jakmeier added 2 commits September 14, 2022 14:44

fix merge test conflicts

c4dad69

fix state viewer merge conflicts

faf7895

near-bulldozer bot merged commit 113f66c into near:master Sep 14, 2022

This was referenced Sep 15, 2022

cherry-pick prefetcher #7621

Merged

Tracking issue: Leftover after Sweatcoin launch #7634

Open

Enable account and access key prefetching by default #7636

Closed

jakmeier mentioned this pull request Sep 30, 2022

Base gas costs for function calls are too high #7741

Open

jakmeier deleted the prefetch-accounts branch November 22, 2022 14:30

feat: prefetch accounts and access keys #7590

feat: prefetch accounts and access keys #7590

Conversation

jakmeier commented Sep 8, 2022

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ekleog left a comment • edited Loading

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

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

jakmeier commented Sep 13, 2022

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

Longarithm commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakmeier commented Sep 13, 2022

Ekleog 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

Choose a reason for hiding this comment

Ekleog 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ekleog left a comment

Choose a reason for hiding this comment

Ekleog left a comment •

edited

Loading