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

fix: 17331: Improve in-memory virtual map mode to run garbage collection for merged copies #17405

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

artemananiev
Copy link
Member

Fix summary:

  • Reworked VirtualNodeCache.filterMutations(), so it can now handle a case, when there is an old unreleased version of the cache
  • Cache mutations are now two-directional lists (prev and next)
  • In VirtualPipeline, every virtual root copy is first compacted (if needed), then either flushed or merged. Previously compaction was only applied to copies, which are eligible to flush
  • A new unit test is provided. A few changes to existing unit tests

Fixes: #17331
Signed-off-by: Artem Ananev [email protected]

@artemananiev artemananiev added this to the v0.59 milestone Jan 16, 2025
@artemananiev artemananiev self-assigned this Jan 16, 2025
@artemananiev artemananiev requested review from a team as code owners January 16, 2025 01:32
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 84.00000% with 16 lines in your changes missing coverage. Please review.

Project coverage is 68.74%. Comparing base (3aac595) to head (e9ffcc0).
Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
...ds/virtualmap/internal/cache/VirtualNodeCache.java 83.33% 3 Missing and 7 partials ⚠️
...ds/virtualmap/internal/merkle/VirtualRootNode.java 85.18% 2 Missing and 2 partials ⚠️
.../virtualmap/internal/pipeline/VirtualPipeline.java 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #17405      +/-   ##
============================================
+ Coverage     67.57%   68.74%   +1.16%     
- Complexity    22130    22589     +459     
============================================
  Files          2588     2610      +22     
  Lines         96654    97232     +578     
  Branches      10099    10130      +31     
============================================
+ Hits          65318    66845    +1527     
+ Misses        27589    26565    -1024     
- Partials       3747     3822      +75     
Files with missing lines Coverage Δ
...rlds/virtualmap/internal/pipeline/VirtualRoot.java 0.00% <ø> (ø)
.../virtualmap/internal/pipeline/VirtualPipeline.java 56.77% <84.61%> (-3.23%) ⬇️
...ds/virtualmap/internal/merkle/VirtualRootNode.java 57.77% <85.18%> (+0.23%) ⬆️
...ds/virtualmap/internal/cache/VirtualNodeCache.java 78.85% <83.33%> (+1.95%) ⬆️

... and 306 files with indirect coverage changes

Impacted file tree graph

Copy link

codacy-production bot commented Jan 16, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+1.37% (target: -1.00%) 94.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (3aac595) 96437 68913 71.46%
Head commit (e9ffcc0) 93366 (-3071) 68001 (-912) 72.83% (+1.37%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#17405) 100 94 94.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

@@ -544,16 +558,13 @@ private void hashFlushMerge() {
if (!copy.isImmutable()) {
break;
}
boolean flushed = false;
compact(copy);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the core of the fix. Every copy, even if not the latest one, is first compacted if needed (if its estimated size exceeds the threshold). Then it can be either flushed (if the size is still more than the threshold after compaction, and the copy is the oldest) or merged (if the size is under threshold), or nothing (if the size exceeds the threshold, but the copy is not the oldest).

… to cover the case. A few other minor fixes

Signed-off-by: Artem Ananev <[email protected]>
Signed-off-by: Artem Ananev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve in-memory virtual map mode to run garbage collection for merged copies
1 participant