-
Notifications
You must be signed in to change notification settings - Fork 552
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
compression: Allocate memory for LZ4_compressEnd #17156
compression: Allocate memory for LZ4_compressEnd #17156
Conversation
Before this change the code assumed that the destination temp. buffer always had enough free space for LZ4F_compressEnd operation. This change checks the free size before calling LZ4F_compressEnd and allocates a new buffer if necessary. Note that the free space required is computed using LZ4F_compressBound with a srcSize of 0. By looking at LZ4F_compressEnd it may appear that we can just check if 12 bytes are free in the destination buffer, whereas LZ4F_compressBound always assumes at least 65443 ie 65535 + block header size (4) + frame footer (4). However, lz4frame docstring for LZ4F_compressBound states that: "When srcSize==0, LZ4F_compressBound() provides an upper bound for LZ4F_flush() and LZ4F_compressEnd() instead." Similar directions are mentioned on LZ4 gh issues, so it is safer to be conservative and possibly allocate the extra bytes.
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46350#018e50da-15de-45b8-8c76-6dc7f78fdfd3 |
output_sz - output_cursor < sz_for_compress_end) { | ||
obuf.trim(output_cursor); | ||
ret.append(std::move(obuf)); | ||
obuf = ss::temporary_buffer<char>(sz_for_compress_end); |
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.
👍
/backport v23.3.x |
/backport v23.2.x |
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.
@abhijat was there an associated issue we were tracking for this?
Before this change the code assumed that the destination temp. buffer always had enough free space for
LZ4F_compressEnd
operation. This change checks the free size before callingLZ4F_compressEnd
and allocates a new buffer if necessary.Note that the free space required is computed using
LZ4F_compressBound
with asrcSize
of 0. By looking atLZ4F_compressEnd
code, it may seem as if we can just check if 12 bytes are free in the destination buffer, whereasLZ4F_compressBound
always assumes at least 65443 ie 65535 + block header size (4) + frame footer (4) possibly resulting in much larger allocation.However, lz4frame docstring for
LZ4F_compressBound
states that:Additionally
LZ4F_compressEnd
may also perform a flush which may need more than 12 bytes.Backports Required
Release Notes