Reduced memory overhead of preparing LZ4-compressed data for server.#110
Merged
traceon merged 3 commits intoClickHouse:masterfrom Nov 15, 2021
Merged
Conversation
Contributor
Do not compress a whole serialized block, but instead only a reasonable-sized chunk. This removes some temporary buffers and reduces memory pressure. Also minor refactoring: - moved all serialization-format code to WireFormat class. - removed CodedOutputStream and CodedInputStream classes.
Enmk
force-pushed
the
memory_optimization_on_send_and_receive_lz4_blocks
branch
from
009b8af to
b148348
Compare
Contributor
|
Could you please provide some numbers of improvements (and absence of degradation)? Like "before vs after" perf tests results, etc. |
traceon
reviewed
Nov 15, 2021
| if (estimated_compressed_buffer_size <= 0) | ||
| throw std::runtime_error("Failed to estimate compressed buffer size, LZ4 error: " + std::to_string(estimated_compressed_buffer_size)); | ||
|
|
||
| compressed_buffer_.resize(estimated_compressed_buffer_size + HEADER_SIZE + EXTRA_COMPRESS_BUFFER_SIZE); |
Contributor
There was a problem hiding this comment.
This is a low hanging fruit for optimization: resize without initialization.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Do not compress a whole serialized block, but instead only a reasonable-sized chunk.
This removes some temporary buffers and reduces memory pressure.
Also minor refactoring:
Memory usage
Test executed against a single block of 5 columns by
INSERTing andSELECTing rows back from the server. Both binaries were built inRelWithDebInfomode.initial- almost right after the program startbefore INSERTing- a moment when the block is prepared in memory, but before actual call toClient::Insertafter INSERTing- right afterClient::Insert, NOTE: original Block is still in memory for validation.after SELECTing- right afterclient.Select("SELECT * FROM ..."), NOTE: original Block is still in memory for validation.Original implementation
Version in this PR
Comparison
Since the original version failed to insert 100M rows, we are going to compare 10M rows memory usage.
As you can see, original implementation peaks to 1117761536 bytes upon insertion, 503341056 of whose are related to the original
Blockresiding in memory, that is 1117761536 - 503341056 = 614420480 bytes (~0.57Gib) of memory used only to INSERT and send data to the server.The modified implementation uses an insignificant amount of extra memory, untraceable with the current approach. Moreover, it is undetectable even upon insertion of 100M rows.
Conclusion
Modified version (presented in this PR) uses
O(1)extra memory, vsO(n)for the original version.