rework accounts hash calc dedup to avoid kernel file error #33195

jeffwashington · 2023-09-08T17:01:06Z

Problem

Summary of Changes

use mmap file instead of BufWriter. The temp file is mmapped for reading anyway.
This avoids the BufWriter api completely.

Fixes #

codecov · 2023-09-08T19:05:59Z

Codecov Report

Merging #33195 (34e37e2) into master (dc6b1eb) will increase coverage by 0.0%.
Report is 3 commits behind head on master.
The diff coverage is 91.8%.

@@           Coverage Diff           @@
##           master   #33195   +/-   ##
=======================================
  Coverage    82.1%    82.1%           
=======================================
  Files         786      786           
  Lines      211596   211608   +12     
=======================================
+ Hits       173777   173843   +66     
+ Misses      37819    37765   -54

jeffwashington · 2023-09-11T13:21:22Z

Perf is about .5s slower in calculating hash.
This is background process, so the difference is not important.

The startup hash calculation actually looks to be faster by around 2-3s.

accounts-db/src/accounts_hash.rs

brooksprumo · 2023-09-11T13:36:22Z

accounts-db/src/accounts_hash.rs

+            let map = unsafe { MmapMut::map_mut(&data) };
+            let map = map.unwrap_or_else(|e| {
+                error!(
+                    "Failed to map the data file (size: {}): {}.\n
+                        Please increase sysctl vm.max_map_count or equivalent for your platform.",
+                    self.capacity, e
+                );
+                std::process::exit(1);
+            });


I know that in append_vec.rs if creating the Mmap fails we'll call exit(), but that feels strange to me. Seems like a panic or expect would work here too. Wdyt?

this looks to be copied from append vec, where stephen wrote this 3 years ago.
Some thoughts:
error messages get logged immediately.
Panics get delayed sometimes and eaten until a thread is joined (IIRC).
Either way it is an error condition. I'm not sure what is best.

Yeah, likely won't matter much, since if we cannot create a new mmap file then it's likely the whole system is borked.

I tend to think a library function shouldn't call exit, but instead assert its invariants. We also get a backtrace with panics, but I doubt it would contain useful information. I'd lean towards doing an expect with a message like "create accounts hashes mmap file".

Not a dealbreaker for me, so I defer to you on this one.

#12213
Here is the source of the copied code.
@sakridge what do you think?

We have some inconsistency.

Here's an info I added a few years ago: #17727

Sometimes we just unwrap the mmap.

solana/accounts-db/src/cache_hash_data.rs

Line 96 in 527a4bb

Ok(unsafe { MmapMut::map_mut(file).unwrap() })

accounts-db/src/accounts_hash.rs

HaoranYi

lgtm

brooksprumo

lgtm

brooksprumo · 2023-09-11T15:38:44Z

Can the title of this PR be updated before merging? (Mainly to remove the 'wip')

* in hash calc, calculate max_inclusive_num_pubkeys * in hash calc, dedup uses mmap files to avoid os panic * as_mut_ptr * remove unsafe code * refactor count in hash files --------- Co-authored-by: HaoranYi <haoran.yi@solana.com> (cherry picked from commit 4dfe62a) # Conflicts: # runtime/src/accounts_hash.rs

…ckport of #33195) (#33204) * rework accounts hash calc dedup to avoid kernel file error (#33195) * in hash calc, calculate max_inclusive_num_pubkeys * in hash calc, dedup uses mmap files to avoid os panic * as_mut_ptr * remove unsafe code * refactor count in hash files --------- Co-authored-by: HaoranYi <haoran.yi@solana.com> (cherry picked from commit 4dfe62a) # Conflicts: # runtime/src/accounts_hash.rs * fix merge conflicts --------- Co-authored-by: Jeff Washington (jwash) <jeff.washington@solana.com>

jeffwashington added the v1.16 PRs that should be backported to v1.16 label Sep 8, 2023

jeffwashington force-pushed the redo_dedup_file branch 3 times, most recently from 150471e to 8103560 Compare September 8, 2023 20:38

jeffwashington marked this pull request as ready for review September 11, 2023 13:21

jeffwashington requested review from HaoranYi and brooksprumo September 11, 2023 13:21

jeffwashington added 2 commits September 11, 2023 08:27

in hash calc, calculate max_inclusive_num_pubkeys

66f6c77

in hash calc, dedup uses mmap files to avoid os panic

c14466c

jeffwashington force-pushed the redo_dedup_file branch from 8103560 to c14466c Compare September 11, 2023 13:28

brooksprumo reviewed Sep 11, 2023

View reviewed changes

accounts-db/src/accounts_hash.rs Show resolved Hide resolved

brooksprumo reviewed Sep 11, 2023

View reviewed changes

as_mut_ptr

7f72ce7

jeffwashington requested a review from brooksprumo September 11, 2023 13:53

brooksprumo reviewed Sep 11, 2023

View reviewed changes

accounts-db/src/accounts_hash.rs Outdated Show resolved Hide resolved

HaoranYi reviewed Sep 11, 2023

View reviewed changes

accounts-db/src/accounts_hash.rs Outdated Show resolved Hide resolved

remove unsafe code

2991a4b

jeffwashington force-pushed the redo_dedup_file branch from 26cd7a8 to 2991a4b Compare September 11, 2023 15:29

refactor count in hash files

34e37e2

jeffwashington requested review from HaoranYi and brooksprumo September 11, 2023 15:36

HaoranYi approved these changes Sep 11, 2023

View reviewed changes

brooksprumo approved these changes Sep 11, 2023

View reviewed changes

jeffwashington changed the title ~~wip: rework accounts hash calc dedup to avoid kernel file error~~ rework accounts hash calc dedup to avoid kernel file error Sep 11, 2023

jeffwashington merged commit 4dfe62a into solana-labs:master Sep 11, 2023

mergify bot mentioned this pull request Sep 11, 2023

v1.16: rework accounts hash calc dedup to avoid kernel file error (backport of #33195) #33204

Merged

steviez mentioned this pull request Sep 20, 2023

panic creating transient file in accounts hash calc #33174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework accounts hash calc dedup to avoid kernel file error #33195

rework accounts hash calc dedup to avoid kernel file error #33195

jeffwashington commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

jeffwashington commented Sep 11, 2023

brooksprumo Sep 11, 2023

jeffwashington Sep 11, 2023

brooksprumo Sep 11, 2023

jeffwashington Sep 11, 2023

jeffwashington Sep 11, 2023

HaoranYi left a comment

brooksprumo left a comment

brooksprumo commented Sep 11, 2023 •

edited

Loading

rework accounts hash calc dedup to avoid kernel file error #33195

rework accounts hash calc dedup to avoid kernel file error #33195

Conversation

jeffwashington commented Sep 8, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

jeffwashington commented Sep 11, 2023

brooksprumo Sep 11, 2023

Choose a reason for hiding this comment

jeffwashington Sep 11, 2023

Choose a reason for hiding this comment

brooksprumo Sep 11, 2023

Choose a reason for hiding this comment

jeffwashington Sep 11, 2023

Choose a reason for hiding this comment

jeffwashington Sep 11, 2023

Choose a reason for hiding this comment

HaoranYi left a comment

Choose a reason for hiding this comment

brooksprumo left a comment

Choose a reason for hiding this comment

brooksprumo commented Sep 11, 2023 • edited Loading

jeffwashington commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

brooksprumo commented Sep 11, 2023 •

edited

Loading