Skip to content
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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ErakhtinB
Copy link
Contributor

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:

  1. Code is formatted: Have you run your code through clang-format to ensure it adheres to the project's coding standards? [Yes|No]
  2. Code is documented: Have you added comments and documentation to your code according to the guidelines in the project's contributing guidelines? [Yes|No]
  3. Self-review: Have you reviewed your own code to ensure it is free of typos, syntax errors, logical errors, and unresolved TODOs or FIXME without linking to an issue? [Yes|No]
  4. Zombienet Tests: Have you ensured that the zombienet tests are passing? Zombienet is a network simulation and testing tool used in this project. It's important to ensure that these tests pass to maintain the stability and reliability of the project. [Yes|No]

@ErakhtinB ErakhtinB linked an issue Aug 27, 2024 that may be closed by this pull request
Comment on lines +18 to +19
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;

Comment on lines +13 to +14
namespace kagome::network {
enum class ZstdStreamCompressorError {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Contributor

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 {
Copy link
Contributor

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

Suggested change
struct ZstdStreamCompressor : public ICompressor {
struct ZstdCompressor : public ICompressor {

Comment on lines +95 to +99
message StateResponseCompressed {
// compressed zstd-stream data representing StateResponse
bytes payload = 1;
}

Copy link
Contributor

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"
Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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)

Copy link
Contributor Author

@ErakhtinB ErakhtinB Sep 2, 2024

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

Copy link
Contributor

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@ErakhtinB ErakhtinB marked this pull request as draft August 28, 2024 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Sync with compression
2 participants