-
Notifications
You must be signed in to change notification settings - Fork 36
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
State Response compressor /decompressor is implemented in zstd, but disabled yet #2195
base: master
Are you sure you want to change the base?
Conversation
virtual outcome::result<std::vector<uint8_t>> compress(std::span<uint8_t> data) = 0; | ||
virtual outcome::result<std::vector<uint8_t>> decompress(std::span<uint8_t> compressedData) = 0; |
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.
virtual outcome::result<std::vector<uint8_t>> compress(std::span<uint8_t> data) = 0; | |
virtual outcome::result<std::vector<uint8_t>> decompress(std::span<uint8_t> compressedData) = 0; | |
virtual outcome::result<Buffer> compress(BufferView data) const = 0; | |
virtual outcome::result<Buffer> decompress(BufferView compressedData) const = 0; |
namespace kagome::network { | ||
enum class ZstdStreamCompressorError { |
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.
namespace kagome::network { | |
enum class ZstdStreamCompressorError { | |
Q_ENUM_ERROR_CODE(ZSTD_ErrorCode) { | |
return ZSTD_getErrorString(static_cast<ZSTD_ErrorCode>(e)); | |
} |
@@ -76,4 +76,4 @@ namespace kagome::network { | |||
} | |||
}; | |||
|
|||
} // namespace kagome::network | |||
} // namespace kagome::network |
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.
Revert, missing empty line at file end
|
||
#include "compressor.h" | ||
namespace kagome::network { | ||
struct ZstdStreamCompressor : public ICompressor { |
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.
ICompressor
doesn't work with streams
struct ZstdStreamCompressor : public ICompressor { | |
struct ZstdCompressor : public ICompressor { |
message StateResponseCompressed { | ||
// compressed zstd-stream data representing StateResponse | ||
bytes payload = 1; | ||
} | ||
|
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.
Unused
@@ -16,6 +16,7 @@ | |||
#include "network/adapters/protobuf.hpp" | |||
#include "network/adapters/uvar.hpp" | |||
#include "network/helpers/message_read_writer.hpp" | |||
#include "network/helpers/compressor/compressor.h" |
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.
Instead of mixing protobuf and compression,
you can compose them
struct ZstdMessageReadWriter : MessageReadWriter {
ZstdMessageReadWriter(MessageReadWriter);
};
struct ProtobufMessageReadWriter {
ProtobufMessageReadWriter(MessageReadWriter);
};
auto state_sync_read_writer =
ProtobufMessageReadWriter(
ZstdMessageReadWriter(
MessageReadWriterUvarint(
stream)));
#include "zstd_error.h" | ||
|
||
namespace kagome::network { | ||
outcome::result<std::vector<uint8_t>> ZstdStreamCompressor::compress(std::span<uint8_t> data) try { |
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.
Why not use ZSTD_compress
/ZSTD_decompress
like uncompressCodeIfNeeded?
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.
Because it won't work, I tried it. The sream compression is used here, you can find the reference to rust code in RFC
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.
They are compatible, I tested.
But stream decompress would be useful 👍
because ZSTD_getFrameContentSize
doesn't work (decompressed size field is empty).
(may move to utils/zstd_decompress.hpp and reuse inside uncompressCodeIfNeeded
)
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.
I tested it directly with polkadot instance running on this commit, which is mentioned in FRC liuchengxu/polkadot-sdk@2556fef#diff-d9656480fbba5813d9d8ff38b5f0661a6019d0b793024a751a1140034fc32747R268
The stream is mentioned there as well. You can try to use ZSTD_compress/ZSTD_decompress in this situation and tell then if it's compatible
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.
compressedData.resize(output.pos); | ||
|
||
return compressedData; | ||
} catch (const std::exception& e) { |
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.
Remove try-catch, zstd functions return error code.
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.
Exceptions are still possible from working with spans/vectors.
Referenced issues
Description of the Change
Some information, as State Response in this PR, can come in compressed way, and has to be decompressed even before parsing into protobuf object. This PR implements compressor interface, zstd stream realization of compressing and decompressing StateResponse, but not enabled during to it's just RFC at the moment
Possible Drawbacks
Checklist Before Opening a PR
Before you open a Pull Request (PR), please make sure you've completed the following steps and confirm by answering 'Yes' to each item: