Plug two leaks around dirty_stores #19025

ryoqun · 2021-08-03T15:53:12Z

Problem

there is regression since #17601

at start-up (verify_snapshot_bank), dirty_stores accumulates Arcs to all shrunken stores because clean_accounts aren't run in verify_snapshot_bank after the initial clean_accounts().
So that keeps mmap leaks. This surge of mmap use here is rather transient; after all shrinking and next clean_accounts(), it's usually munmap()ed en masse.
also the initial clean_accounts() isn't complete as was before. That's because candidate_clean_keys are created from empty dirty_stores. So, ultimately, this leaves more appendvecs than ideally minimized.

BEFORE

note the almost instant dirty_store_processing_us:

[2021-08-03T15:17:40.939120664Z INFO  solana_metrics::metrics] datapoint: clean_accounts collect_delta_keys_us=38365i dirty_store_processing_us=33i accounts_scan=521229i clean_old_rooted=5963348i store_counts=417554i purge_filter=57567i calc_deps=107738i reclaims=69i delta_key_count=1076528i dirty_pubkeys_count=0i total_keys_count=1076528i

Summary of Changes

make dirty_stores population logic more selective.
correctly populate dirty_stores in generate_index().

AFTER

[2021-08-03T15:33:58.804400247Z INFO  solana_metrics::metrics] datapoint: clean_accounts collect_delta_keys_us=35957i dirty_store_processing_us=50367291i accounts_scan=31850539i clean_old_rooted=22471009i store_counts=952847i purge_filter=229714i calc_deps=395295i reclaims=993662i delta_key_count=79001842i dirty_pubkeys_count=79001842i total_keys_count=79001842i

LEFT is before, right is after

Remarks:

Also, I considered following approaches but turned them down:

completely delay populating dirty_stores while booting
- somewhat unknown risks for the introduction of bi-modal behavior; also this patch needs to go v1.7 rather quickly for timely mainnet-beta update
make dirty_stores: DashMap<_, std:sync::Arc<_>>
- generally doesn't work, particularly bad with store recycler (as it recycles when count == 1).

note that there is still very slow leaks, which is occurring even on v1.6.... this pr doesn't address that leak.

ryoqun · 2021-08-03T15:54:57Z

runtime/src/accounts_db.rs

@@ -5233,8 +5235,11 @@ impl AccountsDb {
                );
                let count = store.remove_account(account_info.stored_size, reset_accounts);
                if count == 0 {
-                    self.dirty_stores
-                        .insert((*slot, store.append_vec_id()), store.clone());
+                    // expected_slot is none should exclude write path from flush/shrink/bank drop


kind of hack, but handle_reclaims() code path is literally maze.

ryoqun · 2021-08-03T15:57:01Z

runtime/src/accounts_db.rs

@@ -2254,8 +2254,10 @@ impl AccountsDb {
            if let Some(slot_stores) = self.storage.get_slot_stores(slot) {
                slot_stores.write().unwrap().retain(|_key, store| {
                    if store.count() == 0 {
-                        self.dirty_stores
-                            .insert((slot, store.append_vec_id()), store.clone());
+                        if !is_startup {


admittedly, this is another hack. xD

glad that we have managed to revive the almost-dying flag (_-prefixed) for quick win. :)

ryoqun · 2021-08-03T16:28:03Z

note: there is no test (also ci failed...)

also, here is a crude way to detect leak when shrink_all is running:

# stop the process solana-validator or solana-ledger-tool verify when it starts to display repeated shrink messages.
$ pmap -p 217758 > pmap.bad.maybe-fixed.$(date '+%Yy%mm%dd%Hh%Mm%Ss%Nns')

$ wc -l pmap.bad.maybe-fixed.2021y08m03d0*
   480052 pmap.bad.maybe-fixed.2021y08m03d05h16m26s137495180ns
   546836 pmap.bad.maybe-fixed.2021y08m03d05h16m37s707483373ns
   561374 pmap.bad.maybe-fixed.2021y08m03d05h17m10s011257814ns
   578413 pmap.bad.maybe-fixed.2021y08m03d05h17m48s074379085ns
   400871 pmap.bad.maybe-fixed.2021y08m03d05h43m43s268314673ns
   400867 pmap.bad.maybe-fixed.2021y08m03d05h43m52s882543697ns
   400873 pmap.bad.maybe-fixed.2021y08m03d05h44m47s872216956ns
   400872 pmap.bad.maybe-fixed.2021y08m03d05h45m21s393793566ns
   520249 pmap.bad.maybe-fixed.2021y08m03d06h48m20s214631714ns
   583508 pmap.bad.maybe-fixed.2021y08m03d06h48m30s747138617ns
   615597 pmap.bad.maybe-fixed.2021y08m03d06h48m41s378189396ns
   645764 pmap.bad.maybe-fixed.2021y08m03d06h49m26s753318034ns
   516872 pmap.bad.maybe-fixed.2021y08m03d07h51m44s319430684ns
   573594 pmap.bad.maybe-fixed.2021y08m03d07h51m52s085619175ns
   637344 pmap.bad.maybe-fixed.2021y08m03d07h52m23s614841435ns
   447513 pmap.bad.maybe-fixed.2021y08m03d08h18m08s857074170ns
   447513 pmap.bad.maybe-fixed.2021y08m03d08h18m29s111814438ns
   447514 pmap.bad.maybe-fixed.2021y08m03d08h18m55s404999949ns

sakridge · 2021-08-03T22:35:04Z

runtime/src/accounts_db.rs

@@ -6009,7 +6019,7 @@ impl AccountsDb {
            if pass == 0 {
                // Need to add these last, otherwise older updates will be cleaned
                for slot in &slots {
-                    self.accounts_index.add_root(*slot, false);
+                    self.add_root_to_update_index(*slot, false);


We add any keys which have more than 1 key:

solana/runtime/src/accounts_db.rs

Lines 5904 to 5906 in d8984cb

if !dirty_pubkeys.is_empty() {

self.uncleaned_pubkeys.insert(*slot, dirty_pubkeys);

}

This uses the uncleaned_pubkeys and not dirty_stores which is why the dirty_store time is small, but I don't think we need to perform a clean on keys which only have 1 entry in them, no?

The other case is 0-lamport accounts. We can detect that in generate_index and also add it to the clean set.

yeah, now i view things clearly. I totally agree with you.

ryoqun · 2021-08-04T12:24:38Z

runtime/src/accounts_db.rs

@@ -6009,7 +6019,7 @@ impl AccountsDb {
            if pass == 0 {
                // Need to add these last, otherwise older updates will be cleaned
                for slot in &slots {
-                    self.accounts_index.add_root(*slot, false);
+                    self.add_root_to_update_index(*slot, false);


ryoqun · 2021-08-04T13:37:38Z

superseded by #19041 and #19042

Plug two leaks around dirty_stores

12e488e

ryoqun added the v1.7 label Aug 3, 2021

ryoqun requested review from sakridge and carllin August 3, 2021 15:53

ryoqun commented Aug 3, 2021

View reviewed changes

Revert rename... I'm losing....

9f2b059

sakridge mentioned this pull request Aug 3, 2021

Fix dirty stores #19027

Closed

sakridge reviewed Aug 3, 2021

View reviewed changes

ryoqun commented Aug 4, 2021

View reviewed changes

ryoqun closed this Aug 4, 2021

This was referenced Aug 4, 2021

Clean accounts fixes 1.7 #19045

Closed

Clean within shrink_all_slots #19042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plug two leaks around dirty_stores #19025

Plug two leaks around dirty_stores #19025

ryoqun commented Aug 3, 2021

ryoqun Aug 3, 2021

ryoqun Aug 3, 2021

ryoqun commented Aug 3, 2021 •

edited

Loading

sakridge Aug 3, 2021

sakridge Aug 3, 2021

ryoqun Aug 4, 2021

ryoqun Aug 4, 2021

ryoqun commented Aug 4, 2021 •

edited

Loading

	if !dirty_pubkeys.is_empty() {
	self.uncleaned_pubkeys.insert(*slot, dirty_pubkeys);
	}

Plug two leaks around dirty_stores #19025

Plug two leaks around dirty_stores #19025

Conversation

ryoqun commented Aug 3, 2021

Problem

Summary of Changes

Remarks:

ryoqun Aug 3, 2021

Choose a reason for hiding this comment

ryoqun Aug 3, 2021

Choose a reason for hiding this comment

ryoqun commented Aug 3, 2021 • edited Loading

sakridge Aug 3, 2021

Choose a reason for hiding this comment

sakridge Aug 3, 2021

Choose a reason for hiding this comment

ryoqun Aug 4, 2021

Choose a reason for hiding this comment

ryoqun Aug 4, 2021

Choose a reason for hiding this comment

ryoqun commented Aug 4, 2021 • edited Loading

ryoqun commented Aug 3, 2021 •

edited

Loading

ryoqun commented Aug 4, 2021 •

edited

Loading