Close Zstd Dictionary after execution to avoid any memory leak.

[mgodwan]

Description

While executing some test runs to evaluate the memory usage for indexing path, I found there was an increase in memory used when using Zstd but not with other compression codec implementations.

This PR closes Zstd Dictionary after execution to avoid any memory leak.

Saw the following memory utilization with the existing implementation and after the change:

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[opensearch-trigger-bot]

Compatibility status:

Checks if related components are compatible with change 8255050

Incompatible components

Incompatible components: [https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/security-analytics.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

[reta]

@mgodwan we probable need changelog here (bugfix) since the feature was released in 2.9.0, thank you

[andrross]

@mgodwan Oof, good catch. Does this mean that using zstd compression before this fix will consume all the memory on a host within a relatively short period of time (like hours or days)?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/22928/
• CommitID: fd78242

[codecov]

Codecov Report

Merging #9403 (9ae5684) into main (e1c40b4) will decrease coverage by 0.06%.
The diff coverage is 70.83%.

@@             Coverage Diff              @@
##               main    #9403      +/-   ##
============================================
- Coverage     71.16%   71.11%   -0.06%     
+ Complexity    57409    57381      -28     
============================================
  Files          4776     4776              
  Lines        270711   270713       +2     
  Branches      39574    39574              
============================================
- Hits         192648   192511     -137     
- Misses        61864    62036     +172     
+ Partials      16199    16166      -33     

Files Changed	Coverage Δ
.../index/codec/customcodecs/ZstdCompressionMode.java	86.66% <70.83%> (+0.30%)	⬆️

... and 468 files with indirect coverage changes

[sarthakaggarwal97]

thanks @mgodwan, this is a good catch. LGTM!

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-9403-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5cc73134321fb5c070664f7071014a3f0dedb39e
# Push it to GitHub
git push --set-upstream origin backport/backport-9403-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-9403-to-2.x.

[reta]

@mgodwan could you please port manually to 2.x? thank you

[andrross]

@mgodwan This needs to go in the [Unreleased 2.x] section. I'll submit a quick PR to fix this.

[mgodwan]

Thanks @andrross for fixing this.

[nknize]

Maybe we should use the other ctor? And do this with direct memory instead of using heap?

try (ZstdDictDecompress dictDecompress = new ZstdDictDecompress(ByteBuffer.wrap(bytes.bytes, 0, dictLength))) {

[mgodwan]

The leak is within the native memory and didn't show any issues within the JVM heap. The allocations in native side of code are not freed if this was not closed, and hence lead to the leak.

[austintlee]

Is there still a memory leak? The graph on the right still has the memory utilization climbing.

[mgodwan]

@austintlee I ran a custom workload for 4 hours with the final code which is pushed already and don't see any increase in memory usage (The graph attached in the issue description was for the first cut where PR had not handled the decompression flow)

[bugmakerrrrrr]

@andrross @mgodwan I found that ZstdDictCompress/ZstdDictDecompress has implemented Object#finalize. If GC occurs, the resources will be automatically recycled, so the actual situation is not so serious, right?

[andrross]

@bugmakerrrrrr Indeed, the parent class of ZstdDictCompress/ZstdDictDecompress will release resources upon finalization. Is it still possible that off heap memory could be exhausted before the JVM triggered finalization on these instances? I think I'm still in favor of a patch release to fix this.

[mgodwan]

@bugmakerrrrrr Yes, I also had seen finalize method implicitly calling close. Although, I did not see the cleanup happening without the fix during the long running tests I executed where memory usage climbed from 34G to 52G over a period of 9 hours. During this time, it may possibly impact usage of native memory by other components, such as knn plugin, Lucene fst, etc.

The recommendation is usually to not rely on finalize method due to its unpredictable nature, and can cause issues with the resource exhaustion.

[reta]

@mgodwan all true for finalizers, I am wondering if you do have JVM heap usage charts, my hypothesis is that GC may not be triggering full collections (no heap pressure) and as such, finalizers might not be called.

[dblock]

Thanks for fixing this @mgodwan.

[wbeckler]

What's the next step to get this merged into 2.10?

[reta]

What's the next step to get this merged into 2.10?

@wbeckler It is in 2.x and 2.9.1 already:

• [Backport 2.x] Close Zstd Dictionary after execution to avoid any memory leak. (#9403) #9424
• [Backport 2.9] Close Zstd Dictionary after execution to avoid any memory leak. (#9403) #9425