Skip to content

bank: rewrite status cache #1790

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

Merged
merged 1 commit into from
Jun 17, 2024
Merged

bank: rewrite status cache #1790

merged 1 commit into from
Jun 17, 2024

Conversation

mmcgee-jump
Copy link
Contributor

The status cache has two main issues,

(1) It's not particularly concurrent, and causes slowdowns when running
with highly parallel bank execution.

(2) It has unbounded memory consumption, and can cause out of memory
conditions when it needs to store large numbers of transactions.

It is rewritten in Firedancer for performance. The general design is there are two operations which need to be highly concurrent, insert and query, and everything else is rare and happens between slots and can largely just lock the whole thing.

The base case for insert is optimized to two always-uncontended, and one very lightly contended compare and swap. Query is fully lockless.

The transaction result storage is combined between the snapshot service cache, and the query lookup cache, which more than halves the memory usage and the memory use is fixed up front.

Related to #1770

@mmcgee-jump mmcgee-jump added this to the Frankendancer milestone May 8, 2024
@mmcgee-jump mmcgee-jump requested a review from ptaffet-jump May 8, 2024 19:47
if( FD_UNLIKELY( max_rooted_slots<1UL || max_live_slots<1UL ) ) return 0UL;
if( FD_UNLIKELY( max_live_slots<max_rooted_slots ) ) return 0UL;
if( FD_UNLIKELY( max_txn_per_slot<1UL ) ) return 0UL;
if( FD_UNLIKELY( !fd_ulong_is_pow2( max_live_slots || !fd_ulong_is_pow2( max_txn_per_slot ) ) ) ) return 0UL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if( FD_UNLIKELY( !fd_ulong_is_pow2( max_live_slots || !fd_ulong_is_pow2( max_txn_per_slot ) ) ) ) return 0UL;
if( FD_UNLIKELY( !fd_ulong_is_pow2( max_live_slots ) || !fd_ulong_is_pow2( max_txn_per_slot ) ) ) return 0UL;


ulong txnhash_offset = blockcache->txnhash_offset;
ulong head_hash = FD_LOAD( ulong, query->txnhash+txnhash_offset ) % FD_TXNCACHE_BLOCKCACHE_MAP_CNT;
for( uint head=blockcache->heads[ head_hash ]; head!=UINT_MAX; head=tc->txnpages[ head/FD_TXNCACHE_TXNS_PER_PAGE ].txns[ head%FD_TXNCACHE_TXNS_PER_PAGE ]->blockcache_next ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could consider a macro for tc->txnpages[ head/FD_TXNCACHE_TXNS_PER_PAGE ].txns[ head%FD_TXNCACHE_TXNS_PER_PAGE ] since it shows up several times in places it would be nice if it weren't so long


typedef struct fd_txncache_private_txn fd_txncache_private_txn_t;

struct fd_txncache_private_txnpage {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should mention explicitly here that all the txns refer to the same blockhash

typedef struct fd_txncache_private_txn fd_txncache_private_txn_t;

struct fd_txncache_private_txnpage {
ushort free; /* The number of free txn entries in this page. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Alignment of txns is 4, so you have two bytes of padding here FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't finished understanding it, so I'm not sure this is possible, but what about making a linked list of the pages for a blockhash using these bytes. Then you could get rid of the pages pointer and freeing a list of pages would be O(1).


ulong idx;
for( idx=0UL; idx<tc->root_slots_cnt; idx++ ) {
if( FD_UNLIKELY( tc->root_slots[ idx ]==slot ) ) goto unlock;
Copy link
Contributor

@ptaffet-jump ptaffet-jump May 10, 2024

Choose a reason for hiding this comment

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

Include that re-registering a root slot is a no-op in the function comment

ushort txnpages_per_blockhash_max;

ulong root_slots_cnt; /* The number of root slots being tracked in the below array. */
ulong * root_slots; /* The highest N slots that have been rooted. These slots are
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment about the way this is used? I was definitely expecting something more like a ring buffer with far fewer memmoves, and that initially confused me.

arjain4
arjain4 previously approved these changes Jun 17, 2024
The status cache has two main issues,

 (1) It's not particularly concurrent, and causes slowdowns when running
     with highly parallel bank execution.

 (2) It has unbounded memory consumption, and can cause out of memory
     conditions when it needs to store large numbers of transactions.

It is rewritten in Firedancer for performance.  The general design is
there are two operations which need to be highly concurrent, insert and
query, and everything else is rare and happens between slots and can
largely just lock the whole thing.

The base case for insert is optimized to two always-uncontended, and one
very lightly contended compare and swap.  Query is fully lockless.

The transaction result storage is combined between the snapshot service
cache, and the query lookup cache, which more than halves the memory
usage and the memory use is fixed up front.
@mmcgee-jump mmcgee-jump added this pull request to the merge queue Jun 17, 2024
Merged via the queue into main with commit 01cffb7 Jun 17, 2024
9 checks passed
@mmcgee-jump mmcgee-jump deleted the mmcgee/scachepar4 branch June 17, 2024 19:59
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.

3 participants