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

v2.2: Stops flushing the accounts hash cache mmaps (backport of #5490) #5507

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

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Mar 26, 2025

Problem

mmap flush is very slow, and should be avoided whenever possible. (This also includes msync/fsync.)

We currently flush the accounts hash cache mmaps after creating them. This is unnecessary, and contributes to OS contention.

I originally added flushing the mmaps to fix a bug with accounts hash mismatches, but this was incorrect. Ultimately #5032 correctly identified and fixed the underlying issue.

Here's the accounts hash calc process. The peaks are during full accounts hash calcs. This shows the time spent writing the files themselves, and the time spent flushing the mmaps. It takes 115 seconds to write the files, but 190 seconds to flush the mmaps❗
Screenshot 2025-03-26 at 12 50 02 AM

Summary of Changes

Stop flushing the mmaps.

Note, I intend to backport this to v2.2.

Justification to Backport

Until the lattice hash feature are activated, the accounts hash calculation will still be running. This mmap flush is a big performance hit on all nodes taking snapshots. And the mmap flush was added to fix an issue that turns out it did not fix.

See #5490 (comment) for performance results 🚀


This is an automatic backport of pull request #5490 done by [Mergify](https://mergify.com).

@mergify mergify bot added the conflicts label Mar 26, 2025
@mergify mergify bot requested a review from a team as a code owner March 26, 2025 15:47
Copy link
Author

mergify bot commented Mar 26, 2025

Cherry-pick of 9d662b8 has failed:

On branch mergify/bp/v2.2/pr-5490
Your branch is up to date with 'origin/v2.2'.

You are currently cherry-picking commit 9d662b889.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   accounts-db/src/cache_hash_data_stats.rs

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   accounts-db/src/cache_hash_data.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@brooksprumo
Copy link

@Mergifyio rebase

Copy link
Author

mergify bot commented Mar 26, 2025

rebase

✅ Branch has been successfully rebased

@brooksprumo brooksprumo force-pushed the mergify/bp/v2.2/pr-5490 branch from 3090420 to 0a8def9 Compare March 26, 2025 15:58
@brooksprumo
Copy link

CI is blocked until the version is bumped:

latest channel tag: v2.2.5
current version: v2.2.5
Error: A release has been tagged since your feature branch was last rebased. <current version> must be greater than <latest channel tag>..
        Possible solutions (in the order they should be tried):
        1. rebase your feature branch on the base branch
        2. merge the PR: "Bump Version to ..." once it has passed ci/checks, then rebase
        3. ask for help in #devops on discord
🚨 Error: The command exited with status 1

@brooksprumo
Copy link

@Mergifyio rebase

(cherry picked from commit 9d662b8)

# Conflicts:
#	accounts-db/src/cache_hash_data.rs
Copy link
Author

mergify bot commented Mar 26, 2025

rebase

✅ Branch has been successfully rebased

@brooksprumo brooksprumo force-pushed the mergify/bp/v2.2/pr-5490 branch from 0a8def9 to a6ef354 Compare March 26, 2025 18:04
@brooksprumo brooksprumo requested a review from HaoranYi March 26, 2025 18:05
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.3%. Comparing base (fcba40a) to head (a6ef354).

Additional details and impacted files
@@           Coverage Diff           @@
##             v2.2    #5507   +/-   ##
=======================================
  Coverage    83.3%    83.3%           
=======================================
  Files         800      800           
  Lines      370256   370247    -9     
=======================================
+ Hits       308537   308586   +49     
+ Misses      61719    61661   -58     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

3 participants