-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Close Zstd Dictionary after execution to avoid any memory leak. #9403
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,11 +103,13 @@ | |
|
||
// dictionary compression first | ||
doCompress(bytes, offset, dictLength, cctx, out); | ||
cctx.loadDict(new ZstdDictCompress(bytes, offset, dictLength, compressionLevel)); | ||
try (ZstdDictCompress dictCompress = new ZstdDictCompress(bytes, offset, dictLength, compressionLevel)) { | ||
cctx.loadDict(dictCompress); | ||
|
||
for (int start = offset + dictLength; start < end; start += blockLength) { | ||
int l = Math.min(blockLength, end - start); | ||
doCompress(bytes, start, l, cctx, out); | ||
for (int start = offset + dictLength; start < end; start += blockLength) { | ||
int l = Math.min(blockLength, end - start); | ||
doCompress(bytes, start, l, cctx, out); | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -170,32 +172,33 @@ | |
|
||
// decompress dictionary first | ||
doDecompress(in, dctx, bytes, dictLength); | ||
|
||
dctx.loadDict(new ZstdDictDecompress(bytes.bytes, 0, dictLength)); | ||
|
||
int offsetInBlock = dictLength; | ||
int offsetInBytesRef = offset; | ||
|
||
// Skip unneeded blocks | ||
while (offsetInBlock + blockLength < offset) { | ||
final int compressedLength = in.readVInt(); | ||
in.skipBytes(compressedLength); | ||
offsetInBlock += blockLength; | ||
offsetInBytesRef -= blockLength; | ||
try (ZstdDictDecompress dictDecompress = new ZstdDictDecompress(bytes.bytes, 0, dictLength)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should use the other ctor? And do this with direct memory instead of using heap?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
dctx.loadDict(dictDecompress); | ||
|
||
int offsetInBlock = dictLength; | ||
int offsetInBytesRef = offset; | ||
|
||
// Skip unneeded blocks | ||
while (offsetInBlock + blockLength < offset) { | ||
final int compressedLength = in.readVInt(); | ||
in.skipBytes(compressedLength); | ||
offsetInBlock += blockLength; | ||
offsetInBytesRef -= blockLength; | ||
} | ||
Check warning on line 187 in server/src/main/java/org/opensearch/index/codec/customcodecs/ZstdCompressionMode.java Codecov / codecov/patchserver/src/main/java/org/opensearch/index/codec/customcodecs/ZstdCompressionMode.java#L183-L187
|
||
|
||
// Read blocks that intersect with the interval we need | ||
while (offsetInBlock < offset + length) { | ||
bytes.bytes = ArrayUtil.grow(bytes.bytes, bytes.length + blockLength); | ||
int l = Math.min(blockLength, originalLength - offsetInBlock); | ||
doDecompress(in, dctx, bytes, l); | ||
offsetInBlock += blockLength; | ||
} | ||
|
||
bytes.offset = offsetInBytesRef; | ||
bytes.length = length; | ||
|
||
assert bytes.isValid() : "decompression output is corrupted"; | ||
} | ||
|
||
// Read blocks that intersect with the interval we need | ||
while (offsetInBlock < offset + length) { | ||
bytes.bytes = ArrayUtil.grow(bytes.bytes, bytes.length + blockLength); | ||
int l = Math.min(blockLength, originalLength - offsetInBlock); | ||
doDecompress(in, dctx, bytes, l); | ||
offsetInBlock += blockLength; | ||
} | ||
|
||
bytes.offset = offsetInBytesRef; | ||
bytes.length = length; | ||
|
||
assert bytes.isValid() : "decompression output is corrupted"; | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgodwan This needs to go in the
[Unreleased 2.x]
section. I'll submit a quick PR to fix this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @andrross for fixing this.