Refactors get_snapshot_storages() #3760
Conversation
|
this is fine. But, since it is now called very clean, another alternative is to get all roots less than the cutoff and then get storages based on roots. I imagine that's faster by a lot. Or, we just leave this like it is since by the time master is rolled out we are hopefully ready to skip rewrites finally and enable ancient packing. Not sure what to do. If you're not backporting this, I'd just leave it alone I imagine. |
Yes, this does make an improvement for both clean and snapshots. Thanks for the suggestion!
My initial idea was to backport to v2.1 but not v2.0. Since we're planning to activate skipping rewrites in v2.1, I want to reduce the negative impact of getting the snapshot storages in every iteration of |
|
looks like ci is failing ... Not sure if it is because of the pr or ci flakey I will approve when ci passed. |
Here's some discussion from Discord on the CI issue: https://discord.com/channels/428295358100013066/560503042458517505/1310571536221995068 We'll need PR #3774 to land, and then I can rebase this PR to get the CI fix. |
brooksprumo
force-pushed
the
get-snapshot-storages
branch
from
063e7a5 to
9a5b097
Compare
|
PR #3774 was merged. I've rebased and force-pushed to pull in this fix. No code was changed. |
|
Merging! @HaoranYi, I'm interpreting #3760 (comment) as an approval. Lmk if that's wrong! |
|
@jeffwashington @HaoranYi I'd like to backport to v2.1, since 2.1 and 2.0 are the main ones that'll be impacted by cleaning old storages during |
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
(cherry picked from commit 8c7ae80)
Fine with me. Do you happen to have the updated benchmark time after the new changes? |
The v3 numbers correspond to the code that was merged. Are those the numbers you're looking for? |
|
yeah. lgtm. |
(cherry picked from commit 8c7ae80)
(cherry picked from commit 8c7ae80)
(cherry picked from commit 8c7ae80)
Refactors get_snapshot_storages() (#3760) (cherry picked from commit 8c7ae80) Co-authored-by: Brooks <[email protected]>
Refactors get_snapshot_storages() (#3760) (cherry picked from commit 8c7ae80) Co-authored-by: Brooks <[email protected]>
Problem
AccountsDb::get_snapshot_storages()is due for a refactor. We can speed it up for the common case, and also simplify.Since #3737, we now call
get_snapshot_storages()every time weclean. The observation here is we'll only need about 100 storages (on average), yet the current impl ofget_snapshot_storages()Arc::clone's all the storages, and then filters out the unneeded ones. We can change this to only Arc::clone the useful ones instead.Additionally, the filter step is done in parallel. When we only have 100 storages, the parallel execution does not help. In fact, with a chunk size of 5000, we end up getting zero benefit, but do have to pay the cost of running in the thread pool.
Summary of Changes
Results
I ran this on against mnb and saw good results.
Since
get_snapshot_storages()is called in two places, I wanted to look at the perf results in both.cleanHere, we only need ~100 storages each time. So not Arc::cloning and not using the thread pool really helps. The PR ends up running consistently, and meaningfully, faster:
Full snapshots do need all the storages, so we will end up Arc::cloning almost all the storages. And it's possible the thread pool does help here. For the most part, runtimes are pretty similar. The PR does have a worse worst-case filter time.
Overall, I think the common case of
cleanmakes this change clearly a win. There is maybe a slight slow down for full snapshots, but that code is both infrequent, and in the background, so I don't think it matters much. Additionally, by not using a thread pool, we may reduce resource usage for the system as a whole.