Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring up to date the concurrent accounts benches #34815

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

brooksprumo
Copy link
Contributor

Problem

The Accounts Index has a type to represent reading an entry that is internally self referencing (ReadAccountMapEntry). In #34786, this type has been identified as contributing significantly to memory management overhead. That issue points to #12787 as the source of the type, and this pull-request mentions (and adds code for) benches that justify the changes.

However, running these benches fails because it violates assertions in accounts db:

thread 'main' panicked at accounts-db/src/account_storage.rs:137:9:
assertion failed: self.map.insert(slot,
        AccountStorageReference {
            id: store.append_vec_id(),
            storage: store,
        }).is_none()

This is because we now always use a write cache in AccountsDb, and these benches were written before the write cache was required.

I'd like to use these benches while working on #34786, so these benches need to be updated to run again.

Summary of Changes

Update store_accounts_with_possible_contention() such that it stores accounts using the write cache.

@brooksprumo brooksprumo self-assigned this Jan 17, 2024
Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0e8f2de) 81.7% compared to head (e72887d) 81.7%.
Report is 136 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34815   +/-   ##
=======================================
  Coverage    81.7%    81.7%           
=======================================
  Files         825      825           
  Lines      223249   223250    +1     
=======================================
+ Hits       182609   182610    +1     
  Misses      40640    40640           

@brooksprumo brooksprumo marked this pull request as ready for review January 18, 2024 14:52
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 2, 2024
@brooksprumo brooksprumo removed the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 2, 2024
Copy link
Contributor

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
all in 'benches', so no runtime impact

@brooksprumo brooksprumo merged commit c5aaca4 into solana-labs:master Feb 15, 2024
36 checks passed
@brooksprumo brooksprumo deleted the accounts-benches branch February 15, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants