Moving predictor to WorkingArea to make it thread safe #13706

shubhajeet · 2025-06-17T16:46:10Z

Summary:

We need to move the Predictor to WorkingArea so that it is local to each thread and thus is thread safe.

Test Plan:

It should pass the test case written in ./compression_test.

…omplete test code

Summary: Added compiler options in the stress test. Need to verify it is correct. Need to verify ldb is correct.

hx235 · 2025-06-17T18:47:46Z

util/auto_skip_compressor.h

@@ -33,6 +33,20 @@ class CompressionRejectionProbabilityPredictor {
  size_t window_size_;
 };

+class AutoSkipCompressionContext : public Compressor::WorkingArea {
+ public:
+  explicit AutoSkipCompressionContext(Compressor::ManagedWorkingArea&& wa)


You can call this AutoSkipWorkingArea

hx235 · 2025-06-17T18:52:57Z

util/auto_skip_compressor.cc

    if (prediction <= kProbabilityCutOff) {
      // decide to compress
      return CompressBlockAndRecord(uncompressed_data, compressed_output,
                                    out_compression_type, wa);
    } else {
-      // decide to bypass compression
+      // bypassed compression


Seems like you accidentally reversed some good fix such as this comment. Make sure to check other places too

hx235 · 2025-06-17T18:55:19Z

util/auto_skip_compressor.h

-// Predict rejection probability using a moving window approach
-// This class is not thread safe
+// Predicts Rejection Probability using previous using past data of certain
+// window size


Another accidental revert?

Make sure this PR is rebased on the upstream main after it includes all your previous fixes to auto skip. This is to not accidentally revert your changes.

hx235

I will leave @pdillinger for the final pass. For quick pass, overall design looks right to me except for some small stuff

util/auto_skip_compressor.cc

pdillinger · 2025-06-17T19:01:09Z

util/auto_skip_compressor.cc

@@ -65,27 +65,44 @@ Status AutoSkipCompressorWrapper::CompressBlock(
    return CompressBlockAndRecord(uncompressed_data, compressed_output,
                                  out_compression_type, wa);
  } else {
-    auto prediction = predictor_->Predict();
+    auto predictor_ptr =
+        static_cast<AutoSkipCompressionContext*>(wa->get())->predictor;


If you look at the API commend on CompressBlock,

// The working area is optional and used to optimize repeated compression by // a single thread. ManagedWorkingArea is provided rather than just // WorkingArea so that it can be used only if the `owner` matches expectation. // This could be useful for a Compressor wrapping more than one alternative // underlying Compressor.

Specifically, the working area should only be used if the owner matches expectations. See BuiltinCompressorV2::CompressBlock checking owner() for example.

Also the working area is optional. It is typically provided by the RocksDB table builder but not guaranteed in general.

@pdillinger If the working area is not provided or a working area with wrong owner is provided, does it mean they are not using AutoSkip? Since the predictor is in the working area, we don't have access to any prediction.

Yeah, I think it is OK to provide degraded functionality (always attempt compression) if the WorkingArea is not provided. I might reconsider the exact contract for this in the future.

Some detail: Right now the compressed secondary cache can't efficiently provide a working area because we cannot predict which threads might access the cache, and how many times they might access it. It is perhaps unlikely that AutoSkip would be used with the compressed secondary cache, but it's possible.

The owner() check part is still outstanding also.

pdillinger · 2025-06-17T19:02:32Z

util/auto_skip_compressor.cc

+Compressor::ManagedWorkingArea AutoSkipCompressorWrapper::ObtainWorkingArea() {
+  auto wrap_wa = wrapped_->ObtainWorkingArea();
+  return ManagedWorkingArea(
+      static_cast<WorkingArea*>(


static_cast should be unnecessary

pdillinger · 2025-06-17T19:04:21Z

util/auto_skip_compressor.cc

 Status AutoSkipCompressorWrapper::CompressBlockAndRecord(
    Slice uncompressed_data, std::string* compressed_output,
    CompressionType* out_compression_type, ManagedWorkingArea* wa) {
+  auto wrap_wa =
+      &(static_cast<AutoSkipCompressionContext*>(wa->get())->wrapped);


Can't this function take AutoSkipCompressionContext instead of WorkingArea to avoid so many casts?

facebook-github-bot · 2025-06-17T19:57:16Z

@shubhajeet has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-17T20:39:30Z

@shubhajeet has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-17T20:53:55Z

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

pdillinger

Very close. Resolve the WorkingArea extra checks needed

pdillinger · 2025-06-17T21:51:08Z

util/auto_skip_compressor.cc

@@ -62,14 +62,18 @@ Status AutoSkipCompressorWrapper::CompressBlock(
      "AutoSkipCompressorWrapper::CompressBlock::exploitOrExplore",
      &exploration);
  if (exploration) {
+    auto autoskip_wa = static_cast<AutoSkipWorkingArea*>(wa->get());


We still have three ugly static_cast<AutoSkipWorkingArea*> where I believe one should be sufficient.

util/auto_skip_compressor.cc

pdillinger · 2025-06-17T21:51:52Z

util/auto_skip_compressor.cc

@@ -65,27 +65,44 @@ Status AutoSkipCompressorWrapper::CompressBlock(
    return CompressBlockAndRecord(uncompressed_data, compressed_output,
                                  out_compression_type, wa);
  } else {
-    auto prediction = predictor_->Predict();
+    auto predictor_ptr =
+        static_cast<AutoSkipCompressionContext*>(wa->get())->predictor;


The owner() check part is still outstanding also.

facebook-github-bot · 2025-06-17T22:05:21Z

@shubhajeet has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-17T22:06:52Z

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

facebook-github-bot · 2025-06-17T22:13:33Z

@shubhajeet has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-17T22:15:15Z

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

pdillinger

Looks good, thanks!

facebook-github-bot · 2025-06-18T02:20:31Z

@shubhajeet merged this pull request in 34d8f03.

Summary: **Summary:** We need to move the Predictor to WorkingArea so that it is local to each thread and thus is thread safe. Pull Request resolved: facebook#13706 Test Plan: It should pass the test case written in ./compression_test. Reviewed By: pdillinger Differential Revision: D76836846 Pulled By: shubhajeet fbshipit-source-id: 0d0170baf65f4bb95ba107fec77151e66b8a4449

shubhajeet added 30 commits May 28, 2025 16:25

Addding SimpleMixedCompressor, SimpleMixedCompressionManager, and inc…

de6880d

…omplete test code

Using CompressionWrapper instead

baf2a0f

<Replace this line with a title. Use 1 line only, 67 chars or less>

3ececad

Summary: Added compiler options in the stress test. Need to verify it is correct. Need to verify ldb is correct.

merging with master branch

80bcf10

ldb command seems to be working fine in commandline inspection

2856b2b

using universal counter

d700867

using static counter

f248ab8

Removing the fprint statement used in checking

6904a8e

Trying to avoid warning error on build

35e2158

Trying to avoid warning error on build

fb64e26

SimpleMixed RoundRobinManager test only if ZSTD is linked

547377b

Code cleanup

a0ce6b4

Hoping to fix Status not checked assertion error.

24393e5

setting bottomost compresion as ZSTD

496d630

changed some variable name

1569a0e

Added the compression_type mix

59ad491

Code refactoring as per the reviewers comment

9492245

Moving CompressBlock api

a0fc95d

Fixing the some lines mistakenly deleted

89df03c

Merge branch 'mixed_compressor' into mixed_bench

e67ffd1

Merge branch 'main' into mixed_compressor

2c43d0f

Merge branch 'main' into mixed_bench

4dbd839

Adding flag same value rate

f4e3d62

Avoiding unused type

021d1e8

Merge branch 'mixed_compressor' into mixed_bench

86f60b2

same value always returns initial part

d7bcf15

Moved the block align disable to finalize options function

b30e81f

Merge branch 'mixed_compressor' into mixed_bench

5626e96

adding separate compression_manager flag

1e244cb

Code refactoring

238d6b2

shubhajeet requested review from hx235 and pdillinger June 17, 2025 18:42

hx235 reviewed Jun 17, 2025

View reviewed changes

pdillinger requested changes Jun 17, 2025

View reviewed changes

shubhajeet added 2 commits June 17, 2025 12:42

Adding move constructor and move assignment operator

b5158e9

Renamed AutoSkipCompressionContext -> AutoSkipWorkingArea

ecdf96f

refactoring as per review

1c46eff

pdillinger requested changes Jun 17, 2025

View reviewed changes

Dealing with null working area and working area owner is not this class.

0b223e3

Refactoring

2e8dcfb

shubhajeet self-assigned this Jun 17, 2025

shubhajeet requested a review from pdillinger June 17, 2025 23:52

pdillinger approved these changes Jun 18, 2025

View reviewed changes

facebook-github-bot closed this in 34d8f03 Jun 18, 2025

facebook-github-bot added the Merged label Jun 18, 2025

hx235 mentioned this pull request Jun 18, 2025

Auto skip Compression #13674

Closed

shubhajeet deleted the working_area branch June 26, 2025 21:52

Moving predictor to WorkingArea to make it thread safe #13706

Moving predictor to WorkingArea to make it thread safe #13706

Uh oh!

Conversation

shubhajeet commented Jun 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hx235 Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hx235 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubhajeet Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 17, 2025

Uh oh!

facebook-github-bot commented Jun 17, 2025

Uh oh!

facebook-github-bot commented Jun 17, 2025

Uh oh!

pdillinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 17, 2025

Uh oh!

facebook-github-bot commented Jun 17, 2025

Uh oh!

facebook-github-bot commented Jun 17, 2025

Uh oh!

facebook-github-bot commented Jun 17, 2025

Uh oh!

pdillinger left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 18, 2025

Uh oh!

Uh oh!

hx235 Jun 17, 2025 •

edited

Loading

hx235 left a comment •

edited

Loading

shubhajeet Jun 17, 2025 •

edited

Loading