make PrestoSerializer compression compatible with Presto and add stats #8851

oerling · 2024-02-26T02:30:26Z

Presto uses the uncompressed size as part of checksum, not the compressed size. Presto also does not use the compression result unless it is under 0,8 x the source size. Presto does not require the compressed bit to be set if a compressing exchange codec is in place.

Adds counters to serializers for tracking compression efficacity.

Adds a setting to OutputBufferManager to decide if compression should be attempted for exchange.

Compatibly with Presto, uses uncompressed output if compression yields less then 20% savings. In addition to Presto, skips attempting compression for a number of forthcoming batches if compression was not effective.

Reports the compression input bytes, compressed bytes and bytes for which compression was not attempted in runtimeStats.

netlify · 2024-02-26T02:30:44Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`0fc334f`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65f2a4127b22a60008326ee7

facebook-github-bot · 2024-02-26T02:32:55Z

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng · 2024-03-01T21:39:50Z

velox/common/memory/StreamArena.h

@@ -62,6 +62,17 @@ class StreamArena {
    return pool_;
  }

+  ///


xiaoxmeng · 2024-03-01T21:40:34Z

velox/common/memory/ByteStream.h

@@ -247,6 +247,11 @@ class ByteOutputStream {
  }

  void startWrite(int32_t initialSize) {
+    ranges_.clear();


This is related to this change? Thanks!

xiaoxmeng · 2024-03-07T23:56:52Z

velox/exec/Exchange.h

@@ -105,6 +108,7 @@ class Exchange : public SourceOperator {
  std::vector<std::unique_ptr<SerializedPage>> currentPages_;
  bool atEnd_{false};
  std::default_random_engine rng_{std::random_device{}()};
+  const common::CompressionKind compressionKind_;


Move const member first?

xiaoxmeng · 2024-03-07T23:59:04Z

velox/exec/Exchange.cpp

@@ -122,8 +123,11 @@ RowVectorPtr Exchange::getOutput() {
    auto inputStream = page->prepareStreamForDeserialize();

    while (!inputStream.atEnd()) {
+      serializer::presto::PrestoVectorSerde::PrestoOptions options;


Can we make this a member of Exchange object? Thanks!

xiaoxmeng · 2024-03-07T23:59:27Z

velox/exec/OutputBufferManager.h

@@ -117,6 +117,14 @@ class OutputBufferManager {
  // Returns NULL if task not found.
  std::shared_ptr<OutputBuffer> getBufferIfExists(const std::string& taskId);

+  void enableCompression(common::CompressionKind kind) {
+    compressionKind_ = kind;


Add check that kind is not None?

velox/serializers/PrestoSerializer.cpp

xiaoxmeng · 2024-03-08T00:09:56Z

velox/serializers/PrestoSerializer.cpp

@@ -3493,6 +3500,7 @@ void flushCompressed(
  }

  writeInt32(output, numRows);
+  auto maskOffset = output->tellp();


const auto maskOffset =

velox/serializers/PrestoSerializer.cpp

xiaoxmeng · 2024-03-08T05:50:13Z

velox/serializers/PrestoSerializer.cpp

  const int32_t compressedSize = compressed->length();
  writeInt32(output, uncompressedSize);
+  if (compressedSize > uncompressedSize * 0.8) {
+    writeInt32(output, uncompressedSize);


It looks like the compressed and uncompressed path share many code except the actual buffer write.

velox/serializers/PrestoSerializer.cpp

xiaoxmeng · 2024-03-12T20:51:45Z

velox/serializers/PrestoSerializer.cpp

@@ -77,18 +77,24 @@ int64_t computeChecksum(
    ByteInputStream* source,
    int codecMarker,
    int numRows,
-    int uncompressedSize) {
+    int uncompressedSize,


Shall we use the same type for compressed and uncompressed size? Thanks!

velox/serializers/PrestoSerializer.cpp

xiaoxmeng · 2024-03-12T21:05:17Z

velox/serializers/PrestoSerializer.cpp

 }

-void flushCompressed(
+std::pair<int64_t, int64_t> flushCompressed(


struct FlushSizes { uint64_t compressedSize; uint64_t uncompressedSize; }

velox/serializers/PrestoSerializer.cpp

velox/exec/tests/MultiFragmentTest.cpp

xiaoxmeng · 2024-03-12T21:24:40Z

velox/exec/tests/MultiFragmentTest.cpp

+      makeFlatVector<int32_t>(3'000, [](auto row) { return 1 + row % 3; }),
+  });
+
+  auto test = [&](uint64_t maxBytes, int32_t expectedBatches) {


where is maxBytes used?

facebook-github-bot · 2024-03-13T19:12:58Z

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng

@oerling thanks for the update. It looks pretty good now!

xiaoxmeng · 2024-03-13T20:51:31Z

velox/exec/Exchange.h

@@ -105,6 +110,7 @@ class Exchange : public SourceOperator {
  std::vector<std::unique_ptr<SerializedPage>> currentPages_;
  bool atEnd_{false};
  std::default_random_engine rng_{std::random_device{}()};
+  serializer::presto::PrestoVectorSerde::Options options_;


NYC: maybe we shall consider to have a constructor for serializer::presto::PrestoVectorSerde::Options and many places we initialize the Options with a compression kind.

velox/exec/OutputBufferManager.h

xiaoxmeng · 2024-03-13T20:54:32Z

velox/exec/OutputBufferManager.cpp

+  if (!instance_) {
+    instance_ = std::make_shared<OutputBufferManager>(compressionKind);
+  }
+  return instance_;


Do you want to add a setInstance() API so we can make Prestissimo to use that one?

velox/exec/PartitionedOutput.h

xiaoxmeng · 2024-03-13T20:57:22Z

velox/serializers/PrestoSerializer.cpp

+  return uncompressedSize;
+}
+namespace {
+struct FlushSizes {


Can you comment the structure say if uncompressedSize and compressedSize are the same, then there is no compression.

xiaoxmeng · 2024-03-13T21:02:18Z

velox/exec/tests/MultiFragmentTest.cpp

+    }
+  };
+
+  test(0.7, true);


Can we add a false case by setting its to 1.0? Thanks!

facebook-github-bot · 2024-03-13T22:24:16Z

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebookincubator#8851) Summary: Presto uses the uncompressed size as part of checksum, not the compressed size. Presto also does not use the compression result unless it is under 0,8 x the source size. Presto does not require the compressed bit to be set if a compressing exchange codec is in place. Adds counters to serializers for tracking compression efficacity. Adds a setting to OutputBufferManager to decide if compression should be attempted for exchange. Compatibly with Presto, uses uncompressed output if compression yields less then 20% savings. In addition to Presto, skips attempting compression for a number of forthcoming batches if compression was not effective. Reports the compression input bytes, compressed bytes and bytes for which compression was not attempted in runtimeStats. Reviewed By: xiaoxmeng Differential Revision: D54178975 Pulled By: oerling

facebook-github-bot · 2024-03-14T07:15:52Z

This pull request was exported from Phabricator. Differential Revision: D54178975

facebook-github-bot · 2024-03-14T18:46:32Z

@oerling merged this pull request in 177ff56.

facebookincubator#8851) Summary: Presto uses the uncompressed size as part of checksum, not the compressed size. Presto also does not use the compression result unless it is under 0,8 x the source size. Presto does not require the compressed bit to be set if a compressing exchange codec is in place. Adds counters to serializers for tracking compression efficacity. Adds a setting to OutputBufferManager to decide if compression should be attempted for exchange. Compatibly with Presto, uses uncompressed output if compression yields less then 20% savings. In addition to Presto, skips attempting compression for a number of forthcoming batches if compression was not effective. Reports the compression input bytes, compressed bytes and bytes for which compression was not attempted in runtimeStats. Pull Request resolved: facebookincubator#8851 Reviewed By: xiaoxmeng Differential Revision: D54178975 Pulled By: oerling fbshipit-source-id: 389d7c0d1f9590468c91a405c5c5e5834d29c0da

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 26, 2024

oerling requested a review from Yuhta February 26, 2024 03:19

oerling force-pushed the ser-comp-pr branch 2 times, most recently from c657c51 to 7b68fe4 Compare March 7, 2024 18:50

xiaoxmeng reviewed Mar 8, 2024

View reviewed changes

xiaoxmeng reviewed Mar 12, 2024

View reviewed changes

velox/serializers/PrestoSerializer.cpp Show resolved Hide resolved

xiaoxmeng reviewed Mar 12, 2024

View reviewed changes

oerling force-pushed the ser-comp-pr branch from 7b68fe4 to b018ac5 Compare March 13, 2024 04:32

xiaoxmeng approved these changes Mar 13, 2024

View reviewed changes

oerling force-pushed the ser-comp-pr branch from b018ac5 to a9b6559 Compare March 13, 2024 22:22

oerling force-pushed the ser-comp-pr branch from a9b6559 to 0fc334f Compare March 14, 2024 07:15

facebook-github-bot added the fb-exported label Mar 14, 2024

facebook-github-bot closed this in 177ff56 Mar 14, 2024

facebook-github-bot added the Merged label Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make PrestoSerializer compression compatible with Presto and add stats #8851

make PrestoSerializer compression compatible with Presto and add stats #8851

oerling commented Feb 26, 2024

netlify bot commented Feb 26, 2024 •

edited

Loading

facebook-github-bot commented Feb 26, 2024

xiaoxmeng Mar 1, 2024

xiaoxmeng Mar 1, 2024

xiaoxmeng Mar 7, 2024

xiaoxmeng Mar 7, 2024

xiaoxmeng Mar 7, 2024

xiaoxmeng Mar 8, 2024

xiaoxmeng Mar 8, 2024

xiaoxmeng Mar 12, 2024

xiaoxmeng Mar 12, 2024

xiaoxmeng Mar 12, 2024

facebook-github-bot commented Mar 13, 2024

xiaoxmeng left a comment

xiaoxmeng Mar 13, 2024

xiaoxmeng Mar 13, 2024

xiaoxmeng Mar 13, 2024

xiaoxmeng Mar 13, 2024

facebook-github-bot commented Mar 13, 2024

facebook-github-bot commented Mar 14, 2024

facebook-github-bot commented Mar 14, 2024

make PrestoSerializer compression compatible with Presto and add stats #8851

make PrestoSerializer compression compatible with Presto and add stats #8851

Conversation

oerling commented Feb 26, 2024

netlify bot commented Feb 26, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 13, 2024

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 13, 2024

facebook-github-bot commented Mar 14, 2024

facebook-github-bot commented Mar 14, 2024

netlify bot commented Feb 26, 2024 •

edited

Loading