Close Zstd Dictionary after execution to avoid any memory leak. #9403

mgodwan · 2023-08-16T19:24:54Z

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 · 2023-08-16T19:58:19Z

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 · 2023-08-16T20:08:00Z

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

andrross · 2023-08-16T20:09:19Z

@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 · 2023-08-16T20:13:14Z

Gradle Check (Jenkins) Run Completed with:

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

codecov · 2023-08-16T20:14:19Z

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 · 2023-08-17T12:35:04Z

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 · 2023-08-17T12:42:09Z

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

andrross · 2023-08-17T16:48:44Z

CHANGELOG.md

@@ -50,6 +50,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - [Remote Store] Add Segment download stats to remotestore stats API ([#8718](https://github.com/opensearch-project/OpenSearch/pull/8718))
 - [Remote Store] Add remote segment transfer stats on NodesStats API ([#9168](https://github.com/opensearch-project/OpenSearch/pull/9168))
 - Return 409 Conflict HTTP status instead of 503 on failure to concurrently execute snapshots ([#8986](https://github.com/opensearch-project/OpenSearch/pull/5855))
+- Fix memory leak when using Zstd Dictionary ([#9403](https://github.com/opensearch-project/OpenSearch/pull/9403))


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

Thanks @andrross for fixing this.

nknize · 2023-08-17T17:56:55Z

server/src/main/java/org/opensearch/index/codec/customcodecs/ZstdCompressionMode.java

-                    in.skipBytes(compressedLength);
-                    offsetInBlock += blockLength;
-                    offsetInBytesRef -= blockLength;
+                try (ZstdDictDecompress dictDecompress = new ZstdDictDecompress(bytes.bytes, 0, dictLength)) {


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))) {

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 · 2023-08-17T21:06:10Z

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

…search-project#9403) * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Add changelog Signed-off-by: Mohit Godwani <mgodwan@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com> (cherry picked from commit 5cc7313)

…search-project#9403) * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Add changelog Signed-off-by: Mohit Godwani <mgodwan@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com> (cherry picked from commit 5cc7313) Signed-off-by: Andrew Ross <andrross@amazon.com>

mgodwan · 2023-08-18T14:52:31Z

@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)

…search-project#9403) * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Add changelog Signed-off-by: Mohit Godwani <mgodwan@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com> (cherry picked from commit 5cc7313) Signed-off-by: Andrew Ross <andrross@amazon.com>

…ory leak. (#9403) (#9425) * Close Zstd Dictionary after execution to avoid any memory leak. (#9403) * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Add changelog Signed-off-by: Mohit Godwani <mgodwan@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com> (cherry picked from commit 5cc7313) Signed-off-by: Andrew Ross <andrross@amazon.com> * Add 1.3.13 to bwc versions Signed-off-by: Andrew Ross <andrross@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com> Signed-off-by: Andrew Ross <andrross@amazon.com> Co-authored-by: Mohit Godwani <81609427+mgodwan@users.noreply.github.com>

… (#9424) * Close Dictionary after every execution to avoid any memory leak * Close Dictionary after every execution to avoid any memory leak * Add changelog --------- (cherry picked from commit 5cc7313) Signed-off-by: Mohit Godwani <mgodwan@amazon.com> Signed-off-by: Andrew Ross <andrross@amazon.com> Co-authored-by: Mohit Godwani <81609427+mgodwan@users.noreply.github.com>

bugmakerrrrrr · 2023-08-21T11:36:20Z

@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 · 2023-08-21T16:53:24Z

@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 · 2023-08-22T10:22:21Z

@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 · 2023-08-22T11:59:16Z

@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.

…search-project#9403) * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Add changelog Signed-off-by: Mohit Godwani <mgodwan@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com>

dblock · 2023-08-24T18:52:19Z

Thanks for fixing this @mgodwan.

…search-project#9403) * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Add changelog Signed-off-by: Mohit Godwani <mgodwan@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com>

…search-project#9403) * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Add changelog Signed-off-by: Mohit Godwani <mgodwan@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com> Signed-off-by: Kiran Reddy <kkreddy@amazon.com>

wbeckler · 2023-08-31T16:30:49Z

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

reta · 2023-08-31T17:24:08Z

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

…search-project#9403) * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Add changelog Signed-off-by: Mohit Godwani <mgodwan@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com> Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

…search-project#9403) * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Add changelog Signed-off-by: Mohit Godwani <mgodwan@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com> Signed-off-by: Ivan Brusic <ivan.brusic@flocksafety.com>

…search-project#9403) * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Close Dictionary after every execution to avoid any memory leak Signed-off-by: Mohit Godwani <mgodwan@amazon.com> * Add changelog Signed-off-by: Mohit Godwani <mgodwan@amazon.com> --------- Signed-off-by: Mohit Godwani <mgodwan@amazon.com> Signed-off-by: Shivansh Arora <hishiv@amazon.com>

reta approved these changes Aug 16, 2023

View reviewed changes

sarthakaggarwal97 approved these changes Aug 17, 2023

View reviewed changes

reta merged commit 5cc7313 into opensearch-project:main Aug 17, 2023
13 of 14 checks passed

reta added the backport 2.x Backport to 2.x branch label Aug 17, 2023

opensearch-trigger-bot bot added the backport-failed label Aug 17, 2023

andrross reviewed Aug 17, 2023

View reviewed changes

nknize reviewed Aug 17, 2023

View reviewed changes

nknize mentioned this pull request Aug 17, 2023

[Demote] ZStd compression from GA / LTS to experimental and release a 2.9.1 patch #9422

Closed

dblock mentioned this pull request Sep 8, 2023

Track and report managed/native memory footprint opensearch-project/opensearch-benchmark#369

Open

sarthakaggarwal97 mentioned this pull request Apr 7, 2024

Add hardware-accelerated codecs for DEFLATE and LZ4 opensearch-project/custom-codecs#122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close Zstd Dictionary after execution to avoid any memory leak. #9403

Close Zstd Dictionary after execution to avoid any memory leak. #9403

mgodwan commented Aug 16, 2023 •

edited

Loading

opensearch-trigger-bot bot commented Aug 16, 2023

reta commented Aug 16, 2023 •

edited

Loading

andrross commented Aug 16, 2023

github-actions bot commented Aug 16, 2023

codecov bot commented Aug 16, 2023 •

edited

Loading

sarthakaggarwal97 left a comment

opensearch-trigger-bot bot commented Aug 17, 2023

reta commented Aug 17, 2023

andrross Aug 17, 2023

mgodwan Aug 18, 2023

nknize Aug 17, 2023 •

edited

Loading

mgodwan Aug 18, 2023

austintlee commented Aug 17, 2023

mgodwan commented Aug 18, 2023

bugmakerrrrrr commented Aug 21, 2023

andrross commented Aug 21, 2023

mgodwan commented Aug 22, 2023 •

edited

Loading

reta commented Aug 22, 2023

dblock commented Aug 24, 2023

wbeckler commented Aug 31, 2023

reta commented Aug 31, 2023

Close Zstd Dictionary after execution to avoid any memory leak. #9403

Close Zstd Dictionary after execution to avoid any memory leak. #9403

Conversation

mgodwan commented Aug 16, 2023 • edited Loading

Description

Check List

opensearch-trigger-bot bot commented Aug 16, 2023

Compatibility status:

Incompatible components

Skipped components

Compatible components

reta commented Aug 16, 2023 • edited Loading

andrross commented Aug 16, 2023

github-actions bot commented Aug 16, 2023

Gradle Check (Jenkins) Run Completed with:

codecov bot commented Aug 16, 2023 • edited Loading

Codecov Report

sarthakaggarwal97 left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Aug 17, 2023

reta commented Aug 17, 2023

andrross Aug 17, 2023

Choose a reason for hiding this comment

mgodwan Aug 18, 2023

Choose a reason for hiding this comment

nknize Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

mgodwan Aug 18, 2023

Choose a reason for hiding this comment

austintlee commented Aug 17, 2023

mgodwan commented Aug 18, 2023

bugmakerrrrrr commented Aug 21, 2023

andrross commented Aug 21, 2023

mgodwan commented Aug 22, 2023 • edited Loading

reta commented Aug 22, 2023

dblock commented Aug 24, 2023

wbeckler commented Aug 31, 2023

reta commented Aug 31, 2023

mgodwan commented Aug 16, 2023 •

edited

Loading

reta commented Aug 16, 2023 •

edited

Loading

codecov bot commented Aug 16, 2023 •

edited

Loading

nknize Aug 17, 2023 •

edited

Loading

mgodwan commented Aug 22, 2023 •

edited

Loading