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

Update dashmap to 6.1.0 for cache-aligned shards #5144

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Arrowana
Copy link

@Arrowana Arrowana commented Mar 5, 2025

Problem

DashMap is still used in many places notably Solana accounts-db

There is a new version with some changes which should provide some speedup as presented in the benchmark of this PR
xacrimon/dashmap#303

Summary of Changes

Update to dashmap 6.1.0

Note:

  • Also updated serial_test and governor so there isn't a need for 5.5.3 anymore
  • Raw entry api wants unsafe now
  • I don't know how to measure the impact of the changes, running the accounts-db benches locally gives me wild results on main

Fixes #

@mergify mergify bot requested a review from a team March 5, 2025 00:10
@joncinque joncinque added the CI Pull Request is ready to enter CI label Mar 5, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 5, 2025
@joncinque joncinque added the CI Pull Request is ready to enter CI label Mar 5, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 5, 2025
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

I don't know how to measure the impact of the changes, running the accounts-db benches locally gives me wild results on main

For dependency version bumps, I don't think we have required benches / data to show that the bump improves thing. But, if we suspect that this PR will have a meaningful impact, it could be cool to measure to see.

Brooks or Haoran might be able to lend some insight on the benches and/or whatever else to look into to evaluate any perf improvements

Comment on lines -371 to +372
for (key, entry) in shard.iter().choose_multiple(rng, remaining_samples) {
for (key, entry) in unsafe { shard.iter().map(|bucket| bucket.as_ref()) }
.choose_multiple(rng, remaining_samples)
Copy link

Choose a reason for hiding this comment

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

The code change is fairly small, but in general, probably good to alert someone from AccountsDb team for review.

CC @brooksprumo @HaoranYi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants