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

Uses dashmap for cache_for_accounts_lt_hash #4148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brooksprumo
Copy link

@brooksprumo brooksprumo commented Dec 16, 2024

Problem

The accounts lt hash cache is a HashMap behind a single RwLock. When inserting into the cache, we do a lookup with a read lock, then—if not present—do an insert with a write lock. This can have contention with other replay threads also trying to access/update the cache.

Summary of Changes

Replace the cache's RwLock + HashMap with a DashMap.

Results

I ran this PR vs master (ish), both with the accounts lt hash cli arg enabled. Since upsert into the cache is part of tx processing, we want to reduce latency as much as possible. The upsert is two parts: first, lookup to see if present (read lock only), and second, if not present, insert (with write lock).

brux1 is this PR, brux2 is master.

for lookup, brux1 is red and brux2 is yellow

lookup mean: dashmap is faster on average
mean

lookup max: dashamp has better worst-case lookup times
max


The speedup in lookup (above) is the main win. The time spent inserting is mostly the same.

Here's a graph of the sum of the total inspect time (lookup + insert), which also shows a speed up with this PR.

brux1 is blue, brux2 is purple.

inspect sum

@brooksprumo brooksprumo self-assigned this Dec 16, 2024
@brooksprumo brooksprumo marked this pull request as ready for review December 17, 2024 16:15
@brooksprumo
Copy link
Author

@HaoranYi - requesting your review as an accounts-db expert.
@alessandrod - requesting your review as a performance expert. I want to make sure this isn't misguided/shortsighted/etc!

Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

Nice. I like it.

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