Removes the storage recycler #118

brooksprumo · 2024-03-06T21:10:00Z

Problem

The storage recycler is no longer necessary. It is the cause of this fastboot issue¹, and prevents this fastboot usage².

Summary of Changes

Remove the storage recycler

More information

In the PR that added the recycler³, it mentions two motivations:

file/mmap creation can be expensive - 100s of ms
mmap deletion causes a flush to disk, which is unnecessary if the data is not needed

Now that we have the accounts write cache, only roots are written to storages. IOW, we don't have storages for slots on a minority fork that got discarded. We also don't "append" to (non-ancient) append vecs anymore. I think this means we won't ever flush a non-root, and we won't have unflushed data in a storage.

I posted graphs of metrics over on Discord⁴ that showed performance metrics w.r.t. dropping storages and creating storages. There are more metrics at the end of this description too. Tl;dr: occasionally this PR takes a while to create a new mmap file. Mmap deletion seems to be faster with this PR.

I think this is an overall win. The code gets simpler. We unlock a desirable use-case. And we can remove 'atomic' from the AccountStorageEntry slot and id fields. Creating an mmap did get slower, once out of an hour window. This is in the background, and the overall flush/clean/shrink metrics did not seem to be negatively impacted.

More Metrics

There are two nodes in these metrics
- brux is this PR, aka recycler OFF
- dev7 is master (the commit prior to this PR), aka recycler ON
This is on mnb, over 1 hour

The number of dropped storages, with the values summed. Both nodes appear the same.

Time to create storages, during shrink. Also the sum. (I have 'min' and 'max' graphs too, but they look similar.) Presumably this is when recycler ON should be a big win. We do see that this does spike when the recycler is OFF.

Time to drop storages, during shrink, summed. Basically zero.

Time to drop storages, during clean, summed. With recycler OFF, this is almost always zero. With recycler ON, this periodically takes 80-120 ms. Interesting...

Looking at clean and shrink overall, the total times for each appear to be mostly the same.

brooksprumo · 2024-03-06T21:24:06Z

accounts-db/src/ancient_append_vecs.rs

+        // NOTE: The recycler has been removed.  Creating this many extra storages is no longer
+        // necessary, but also does no harm either.
+        const MAX_RECYCLE_STORES: usize = 1000;


The MAX_RECYCLE_STORES is now only used in this test. In an effort to create a small PR, I have not updated the test here.

brooksprumo · 2024-03-06T21:25:04Z

runtime/src/snapshot_minimizer.rs

-        let (_, drop_or_recycle_stores_measure) = measure!(
-            self.accounts_db()
-                .drop_or_recycle_stores(dead_storages, &self.accounts_db().shrink_stats),
-            "drop or recycle stores"
-        );
-        info!("{drop_or_recycle_stores_measure}");


The drop_or_recycle_stores() function has been removed, so we call drop explicitly now.

brooksprumo · 2024-03-06T21:33:26Z

accounts-db/src/accounts_db.rs

@@ -3957,9 +3854,15 @@ impl AccountsDb {
            );
        }

-        self.drop_or_recycle_stores(dead_storages, stats);


This function was removed, so we call drop directly now.

brooksprumo · 2024-03-06T21:35:04Z

accounts-db/src/accounts_db.rs

-    pub fn drop_or_recycle_stores(
-        &self,
-        dead_storages: Vec<Arc<AccountStorageEntry>>,
-        stats: &ShrinkStats,
-    ) {
-        let mut recycle_stores_write_elapsed = Measure::start("recycle_stores_write_time");
-        let mut recycle_stores = self.recycle_stores.write().unwrap();
-        recycle_stores_write_elapsed.stop();
-
-        let mut drop_storage_entries_elapsed = Measure::start("drop_storage_entries_elapsed");
-        if recycle_stores.entry_count() < MAX_RECYCLE_STORES {
-            recycle_stores.add_entries(dead_storages);
-            drop(recycle_stores);
-        } else {
-            self.stats
-                .dropped_stores
-                .fetch_add(dead_storages.len() as u64, Ordering::Relaxed);
-            drop(recycle_stores);
-            drop(dead_storages);
-        }
-        drop_storage_entries_elapsed.stop();
-        stats
-            .drop_storage_entries_elapsed
-            .fetch_add(drop_storage_entries_elapsed.as_us(), Ordering::Relaxed);
-        stats
-            .recycle_stores_write_elapsed
-            .fetch_add(recycle_stores_write_elapsed.as_us(), Ordering::Relaxed);
-    }


When not recycling, this function boils down to:

drop the storages

update stats

We inline this into the one real caller above.

Yah. Probably we can save 200us.

brooksprumo · 2024-03-06T21:36:06Z

accounts-db/src/accounts_db.rs

@@ -5831,32 +5612,26 @@ impl AccountsDb {
            .safety_checks_elapsed
            .fetch_add(safety_checks_elapsed.as_us(), Ordering::Relaxed);

-        let mut total_removed_storage_entries = 0;


This is a duplicate of num_stored_slots_removed, so we just use that one instead for the stats.

brooksprumo · 2024-03-06T21:37:04Z

accounts-db/src/accounts_db.rs

+        self.stats
+            .dropped_stores
+            .fetch_add(num_stored_slots_removed as u64, Ordering::Relaxed);


This stat update used to live in recycle_slot_stores() (which was removed).

codecov-commenter · 2024-03-06T23:02:41Z

Codecov Report

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

Project coverage is 81.8%. Comparing base (c161351) to head (e7fbb6c).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master     #118     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         838      838             
  Lines      225947   225700    -247     
=========================================
- Hits       184971   184717    -254     
- Misses      40976    40983      +7

jeffwashington · 2024-03-07T16:06:00Z

First pass review looks good to me. I'm going to use a finer tooth comb and go over it again and think about this for a while.

HaoranYi · 2024-03-07T19:21:20Z

Overall looks good.

From the stats on mainnet, it looks like the recycle store is almost always full with 1K entry. I guess this means that not many of the entries in the store got recycled?

Do we have any other metric that directly count how many stores are recycled?

jeffwashington

lgtm
super excited to reduce complexity and get rid of this whole concept. It always attacks suspicion and will simplify the model of how to think about storages.

brooksprumo · 2024-03-07T21:14:32Z

Do we have any other metric that directly count how many stores are recycled?

Here's metrics from a 15 minute interval for all of the canaries running on mnb:

The spikes are likely for each run of flush/clean/shrink, every 100 or so slots.

HaoranYi · 2024-03-07T22:21:14Z

Do we have any other metric that directly count how many stores are recycled?

Here's metrics from a 15 minute interval for all of the canaries running on mnb:

The spikes are likely for each run of flush/clean/shrink, every 100 or so slots.

Ok. Looks like we don't recycle much, ie 10%.

HaoranYi · 2024-03-07T22:23:30Z

accounts-db/src/accounts_db.rs

        // Backing mmaps for removed storages entries explicitly dropped here outside
        // of any locks
+        let mut drop_storage_entries_elapsed = Measure::start("drop_storage_entries_elapsed");


nit: measure!(...) as we did for other places.

Yep, I could do that. I was trying to keep any extraneous changes to a minimum. Since it's not a correctness issue, I'd prefer to not tackle that change here in this PR though.

HaoranYi

lgtm.

brooksprumo · 2024-03-07T22:30:42Z

@sakridge Requesting your review to ensure there's not any other gotchas/corner cases I've missed. Since you added the recycler, I figured your input would be valuable. I'm happy to look up any additional metrics too.

sakridge · 2024-03-10T22:09:47Z

Now that we have the accounts write cache, only roots are written to storages. IOW, we don't have storages for slots on a minority fork that got discarded.

Ideally all slots are roots or a very high percentage, so I'm not sure how having the write cache only write roots should change much in terms of file creation assuming we still create new files for each slot.

I think we do write the whole slot to a single file now instead of potentially multiple files as before, so maybe that would reduce the average number of files created.

brooksprumo · 2024-03-11T12:41:13Z

[..] I'm not sure how having the write cache only write roots should change much in terms of file creation assuming we still create new files for each slot.

My understanding was that before the write cache was added, append vecs were created for every slot, before rooting. Yes, most would get rooted, but not all. So some append vecs would get dropped sooner because their slot was pruned due to being on a minority fork.

I think we do write the whole slot to a single file now instead of potentially multiple files as before, so maybe that would reduce the average number of files created.

Yes, now we write-all instead of append. If there is only a single account path, then I think the number of storages would be the same. With multiple account path, then yes, before there could be multiple storages per slot. Combined with the part above, dropped slots on a minority fork could drop more storages if there were many account paths.

brooksprumo · 2024-03-11T19:19:22Z

I've updated the PR's description and added a bunch of metrics under the "More Metrics" details dropdown.

sakridge

Ok, from the metrics seems to be not helping much given the save in complexity and the drop overhead. Thanks adding those additional ones.

brooksprumo · 2024-03-11T19:38:25Z

Ok, from the metrics seems to be not helping much given the save in complexity and the drop overhead.

I'll keep monitoring these metrics going forward to see if there are any regressions.

brooksprumo self-assigned this Mar 6, 2024

brooksprumo force-pushed the recycle/remove branch from 3857c26 to 2f6bd0c Compare March 6, 2024 21:27

Removes the storage recycler

e7fbb6c

brooksprumo force-pushed the recycle/remove branch from 2f6bd0c to e7fbb6c Compare March 6, 2024 21:30

brooksprumo commented Mar 6, 2024

View reviewed changes

This was referenced Mar 6, 2024

Removes atomic-ness from AccountStorageEntry id and slot fields #119

Merged

Removes holding storages in AccountsHashVerifier for fastboot #120

Merged

brooksprumo marked this pull request as ready for review March 6, 2024 22:15

brooksprumo requested review from HaoranYi and jeffwashington March 7, 2024 14:11

jeffwashington approved these changes Mar 7, 2024

View reviewed changes

brooksprumo requested a review from sakridge March 7, 2024 22:19

HaoranYi reviewed Mar 7, 2024

View reviewed changes

HaoranYi approved these changes Mar 7, 2024

View reviewed changes

sakridge approved these changes Mar 11, 2024

View reviewed changes

brooksprumo merged commit 5c1df15 into anza-xyz:master Mar 11, 2024
35 checks passed

brooksprumo deleted the recycle/remove branch March 11, 2024 19:38

This was referenced Mar 21, 2024

Fastboot fails if the node crashes while archiving a full snapshot solana-labs/solana#35367

Closed

Removes AccountsFile::is_recyclable() #359

Merged

codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024

Removes the storage recycler (anza-xyz#118)

cfe6868

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes the storage recycler #118

Removes the storage recycler #118

brooksprumo commented Mar 6, 2024 •

edited

Loading

brooksprumo Mar 6, 2024

brooksprumo Mar 6, 2024

brooksprumo Mar 6, 2024

brooksprumo Mar 6, 2024

HaoranYi Mar 7, 2024

brooksprumo Mar 6, 2024

brooksprumo Mar 6, 2024

codecov-commenter commented Mar 6, 2024

jeffwashington commented Mar 7, 2024

HaoranYi commented Mar 7, 2024

jeffwashington left a comment

brooksprumo commented Mar 7, 2024

HaoranYi commented Mar 7, 2024

HaoranYi Mar 7, 2024

brooksprumo Mar 7, 2024

HaoranYi left a comment

brooksprumo commented Mar 7, 2024

sakridge commented Mar 10, 2024

brooksprumo commented Mar 11, 2024

brooksprumo commented Mar 11, 2024

sakridge left a comment

brooksprumo commented Mar 11, 2024

Removes the storage recycler #118

Removes the storage recycler #118

Conversation

brooksprumo commented Mar 6, 2024 • edited Loading

Problem

Summary of Changes

More information

Footnotes

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

codecov-commenter commented Mar 6, 2024

Codecov Report

jeffwashington commented Mar 7, 2024

HaoranYi commented Mar 7, 2024

jeffwashington left a comment

Choose a reason for hiding this comment

brooksprumo commented Mar 7, 2024

HaoranYi commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoranYi left a comment

Choose a reason for hiding this comment

brooksprumo commented Mar 7, 2024

sakridge commented Mar 10, 2024

brooksprumo commented Mar 11, 2024

brooksprumo commented Mar 11, 2024

sakridge left a comment

Choose a reason for hiding this comment

brooksprumo commented Mar 11, 2024

brooksprumo commented Mar 6, 2024 •

edited

Loading