feat:Support multi-threaded asynchronous data upload to object storage. by weixiuli · Pull Request #14472 · facebookincubator/velox

weixiuli · 2025-08-14T09:51:26Z

#14471
Support multi-threaded asynchronous data upload to object storage.

This PR also adds a benchmark named S3AsyncUploadBenchmark, the output of benchmark show below :

============================================================================
[...]/benchmark/S3AsyncUploadBenchmark.cpp     relative  time/iter   iters/s
============================================================================
sync_upload_4M                                             48.82ms     20.48
async_upload_4M                                 108.32%    45.07ms     22.19
sync_upload_8M                                             79.81ms     12.53
async_upload_8M                                 104.41%    76.44ms     13.08
sync_upload_16M                                           138.74ms      7.21
async_upload_16M                                117.11%   118.47ms      8.44
sync_upload_32M                                           260.98ms      3.83
async_upload_32M                                170.03%   153.49ms      6.52
sync_upload_64M                                           509.86ms      1.96
async_upload_64M                                247.14%   206.30ms      4.85
sync_upload_128M                                          998.46ms      1.00
async_upload_128M                               284.82%   350.56ms      2.85
sync_upload_256M                                             2.00s   499.29m
async_upload_256M                               316.53%   632.76ms      1.58
sync_upload_512M                                             4.01s   249.50m
async_upload_512M                               339.35%      1.18s   846.68m
sync_upload_1024M                                            7.84s   127.62m
async_upload_1024M                              342.67%      2.29s   437.32m
sync_upload_2048M                                           16.40s    60.99m
async_upload_2048M                              332.71%      4.93s   202.93m

We used this PR on spark +gluten+ velox incubator-gluten and set the hive.s3.uploadPartAsync to be true in our environment, and the average write performance improved by 85%.

Before this PR: Total Time Across All Tasks: 64.3 h

After this PR: Total Time Across All Tasks: 32.4 h

netlify · 2025-08-14T09:51:31Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`413eb73`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/68ca60f85c4ee100089ac2f7

jinchengchenghh · 2025-08-15T08:57:30Z

What's the config when you got this performance?

and the average write performance improved by 85%.

weixiuli · 2025-08-18T02:14:30Z

What's the config when you got this performance?

and the average write performance improved by 85%.

We set the hive.s3.uploadPartAsync to true in our environment.

JkSelf · 2025-08-20T02:48:35Z

@weixiuli Thanks for your optimization.
Could you please add a benchmark to your code change to demonstrate the performance improvement?

weixiuli · 2025-08-25T08:22:53Z

@JkSelf @jinchengchenghh Thanks for your review, PTAL.

JkSelf

Thanks for your work. Leaving some comments.

velox/connectors/hive/storage_adapters/s3fs/S3Config.h

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.h

JkSelf · 2025-08-26T06:35:39Z

@weixiuli Please help to resolve the conflict. Thanks.

weixiuli · 2025-09-01T09:28:46Z

@JkSelf @jinchengchenghh @majetideepak Could you help to review this PR? Thanks.

FelixYBW · 2025-09-03T07:02:24Z

@pedroerp could you help to review the PR? @weixiuli confirmed the PR can boost performance in production environment.

weixiuli · 2025-09-08T09:09:54Z

@pedroerp could you help to review the PR? thanks.

JkSelf

@weixiuli Thanks for your fix. Left two comments.

velox/connectors/hive/storage_adapters/s3fs/S3Config.h

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.h

majetideepak

@weixiuli Did you evaluate the S3 Transfer Manager?

weixiuli · 2025-09-10T03:01:11Z

@weixiuli Did you evaluate the S3 Transfer Manager?

Yes, velox may bypass local disk and directly use TransferManager for parallel uploads to S3, but it requires implementing a custom sink connector that feeds in-memory serialized Velox data into TransferManager’s multipart upload API. This design is a more complex one, while this PR solves the problem of asynchronous upload through a small memory buffle, with less modification and significant performance improvement.

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp

majetideepak · 2025-09-11T17:19:05Z

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp

+
+  // Upload the part asynchronously.
+  void uploadPartAsync(const std::string_view part) {
+    maxConcurrentUploadNum_->wait();


What is the purpose of this wait?

maxConcurrentUploadNum_ is a semaphore and controls the concurrency of asynchronous uploads to
S3 for each S3WriteFile, preventing excessive memory usage.

I don't understand the control flow here. The futures are run during close(). Don't we end up adding more tasks than maxConcurrentUploadNum_ at this point?

I don't understand the control flow here. The futures are run during close(). Don't we end up adding more tasks than maxConcurrentUploadNum_ at this point?

@majetideepak The maxConcurrentUploadNum_ only limits the concurrency of asynchronous uploads within each S3WriteFile to prevent excessive memory usage. If multiple S3WriteFile instances exist, more upload tasks can still run concurrently , but it should be less than or equal to hive.s3.upload-threads.

velox/connectors/hive/storage_adapters/s3fs/S3Config.h

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp

majetideepak · 2025-09-11T17:31:03Z

velox/connectors/hive/storage_adapters/s3fs/benchmark/S3FileSystemBenchmark.cpp

+DEFINE_BENCHMARKS(2048)
+} // namespace
+
+int main(int argc, char** argv) {


What is the output of this benchmark?

The benchmark output is same as the description of the PR.

weixiuli · 2025-09-15T01:58:08Z

cc @majetideepak

JkSelf

Thanks for your updating. Leaving some nits.

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp

JkSelf · 2025-09-15T03:25:21Z

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp

+            s3Config->uploadThreads(),
+            std::make_shared<folly::NamedThreadFactory>("upload-thread"));
+      }
+    } else {


Could we remove the else branch since we've already initialized it to null in the parameter definition?

Since uploadThreadPool_ is declared as a static member of S3WriteFile::Impl, we need to add an else branch to handle the case where hive.s3.upload-part-async changes from true to false in benchmarks and unit tests. In this branch, we should reset uploadThreadPool_, while keeping maxConcurrentUploadNum_ unchanged (i.e., no reset needed).

Since tests can run in parallel, this can lead to non-deterministic failures.
Since uploadThreadPool_ is static across different filesystem instances, we have a race condition here.
Can we keep it non-static?

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp

velox/connectors/hive/storage_adapters/s3fs/S3Config.h

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

jinchengchenghh · 2025-09-15T08:43:36Z

velox/connectors/hive/storage_adapters/s3fs/S3Config.h

    kRetryMode,
    kUseProxyFromEnv,
    kCredentialsProvider,
+    KUploadPartAsync,


KUploadPartAsync -> KPartUploadAsync, matching the kPartUploadSize

jinchengchenghh · 2025-09-15T08:43:59Z

velox/connectors/hive/storage_adapters/s3fs/S3Config.h

    return config_.find(Keys::kCredentialsProvider)->second;
  }

+  bool uploadPartAsync() const {


velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.h

velox/connectors/hive/storage_adapters/s3fs/benchmark/S3FileSystemBenchmark.cpp

weixiuli · 2025-09-16T01:29:38Z

cc @JkSelf @jinchengchenghh @majetideepak

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.h

velox/connectors/hive/storage_adapters/s3fs/benchmark/CMakeLists.txt

velox/connectors/hive/storage_adapters/s3fs/benchmark/S3FileSystemBenchmark.cpp

velox/connectors/hive/storage_adapters/s3fs/tests/S3FileSystemTest.cpp

velox/connectors/hive/storage_adapters/s3fs/S3Config.h

JkSelf

LGTM. Thanks for your fix.

velox/connectors/hive/storage_adapters/s3fs/benchmark/CMakeLists.txt

weixiuli · 2025-09-17T06:44:09Z

Thanks @JkSelf cc @jinchengchenghh @majetideepak

weixiuli · 2025-09-23T02:01:19Z

@majetideepak @jinchengchenghh could you help to review the PR again? thanks.

majetideepak · 2025-09-23T09:32:17Z

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp

+            s3Config->uploadThreads(),
+            std::make_shared<folly::NamedThreadFactory>("upload-thread"));
+      }
+    } else {


Since tests can run in parallel, this can lead to non-deterministic failures.
Since uploadThreadPool_ is static across different filesystem instances, we have a race condition here.
Can we keep it non-static?

majetideepak · 2025-09-23T09:39:51Z

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp

+    // Only the last part can be less than partUploadSize_.
+    VELOX_CHECK(isLast || (!isLast && (part.size() == partUploadSize_)));
+    auto uploadPartSync = [&](const std::string_view partData) {
+      Aws::S3::Model::CompletedPart completedPart =


use auto?

majetideepak · 2025-09-23T09:52:34Z

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp

+
+  // Upload the part asynchronously.
+  void uploadPartAsync(const std::string_view part) {
+    maxConcurrentUploadNum_->wait();


I don't understand the control flow here. The futures are run during close(). Don't we end up adding more tasks than maxConcurrentUploadNum_ at this point?

PingLiuPing · 2025-10-08T09:45:50Z

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.h

 /// and creates the object.
 /// https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html
 /// https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/s3fs.cc
 /// S3WriteFile is not thread-safe.


How did you resolve this issue? Thanks.

How did you resolve this issue? Thanks.

The current modification of the pr is an optimization of the internal processing logic of S3WriteFile. We control uploadState_.completedParts through uploadStateMutex_ and ensure that The list of parts should be in ascending order when it is closed. Currently, the internal processing of S3WriteFile is thread-safe.

stale · 2026-01-08T03:15:38Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

FelixYBW · 2026-01-27T02:44:28Z

Using SF2500 TPCDS table, performance is improved by 10%. CPU% is improved obviously:
Without the PR

With the PR:

majetideepak · 2026-01-27T19:09:55Z

@FelixYBW Some of the review comments have not been addressed.
I also think we should probably use the existing connectorIoExecutor instead of creating another executor pool per Writefile

zhztheplayer · 2026-02-06T17:18:36Z

@weixiuli Would you rebase the PR? I am picking it back for internal testing.

weixiuli requested a review from majetideepak as a code owner August 14, 2025 09:51

meta-cla 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 Aug 14, 2025

weixiuli force-pushed the multi-threaded-s3FileSystem branch 3 times, most recently from 0120710 to 8cba9e9 Compare August 14, 2025 11:06

weixiuli force-pushed the multi-threaded-s3FileSystem branch 2 times, most recently from 2d6a94e to 9456784 Compare August 26, 2025 01:32

JkSelf reviewed Aug 26, 2025

View reviewed changes

weixiuli force-pushed the multi-threaded-s3FileSystem branch from 9456784 to 61dcd92 Compare August 28, 2025 02:50

weixiuli changed the title ~~Support multi-threaded asynchronous data upload to object storage.~~ feat:Support multi-threaded asynchronous data upload to object storage. Aug 28, 2025

weixiuli force-pushed the multi-threaded-s3FileSystem branch 2 times, most recently from 2a07c32 to a1217ff Compare September 1, 2025 02:06

weixiuli requested a review from JkSelf September 1, 2025 09:37

weixiuli force-pushed the multi-threaded-s3FileSystem branch from a1217ff to c697ff5 Compare September 3, 2025 13:09

JkSelf reviewed Sep 9, 2025

View reviewed changes

velox/connectors/hive/storage_adapters/s3fs/S3Config.h Outdated Show resolved Hide resolved

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.h Outdated Show resolved Hide resolved

majetideepak requested changes Sep 9, 2025

View reviewed changes

weixiuli added 3 commits September 10, 2025 07:34

feat:Support multi-threaded asynchronous data upload to object storage.

54d2d1a

Add S3FileSystemBenchmark

44204c6

Address comments

904b0ea

majetideepak reviewed Sep 10, 2025

View reviewed changes

velox/connectors/hive/storage_adapters/s3fs/S3WriteFile.cpp Outdated Show resolved Hide resolved

weixiuli force-pushed the multi-threaded-s3FileSystem branch from 5576747 to c4daa54 Compare September 11, 2025 06:57

majetideepak reviewed Sep 11, 2025

View reviewed changes

Address comments

bf5b766

JkSelf reviewed Sep 15, 2025

View reviewed changes

jinchengchenghh reviewed Sep 15, 2025

View reviewed changes

weixiuli added 2 commits September 15, 2025 09:01

Address comments

2a9c10b

Address commnets

9573e78

JkSelf reviewed Sep 16, 2025

View reviewed changes

velox/connectors/hive/storage_adapters/s3fs/S3Config.h Show resolved Hide resolved

Address comments

2faf0f4

weixiuli force-pushed the multi-threaded-s3FileSystem branch from 6589956 to 2faf0f4 Compare September 16, 2025 09:50

JkSelf approved these changes Sep 17, 2025

View reviewed changes

velox/connectors/hive/storage_adapters/s3fs/benchmark/CMakeLists.txt Outdated Show resolved Hide resolved

Address comments

413eb73

majetideepak reviewed Sep 23, 2025

View reviewed changes

PingLiuPing reviewed Oct 8, 2025

View reviewed changes

GregoryKimball mentioned this pull request Oct 28, 2025

[cuDF] Enable use of Velox-managed IO with GPU TableScan #15282

Closed

stale bot added the stale label Jan 8, 2026

stale bot closed this Jan 22, 2026

majetideepak reopened this Jan 27, 2026

stale bot removed the stale label Jan 27, 2026

FelixYBW mentioned this pull request Feb 7, 2026

[VL] useful Velox PRs not merged into upstream apache/gluten#11585

Open

weixiuli mentioned this pull request Feb 25, 2026

feat: Support multi-threaded asynchronous data upload to object storage bytedance/bolt#264

Merged

10 tasks

Conversation

weixiuli commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

jinchengchenghh commented Aug 15, 2025

Uh oh!

weixiuli commented Aug 18, 2025

Uh oh!

JkSelf commented Aug 20, 2025

Uh oh!

weixiuli commented Aug 25, 2025

Uh oh!

JkSelf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JkSelf commented Aug 26, 2025

Uh oh!

weixiuli commented Sep 1, 2025

Uh oh!

FelixYBW commented Sep 3, 2025

Uh oh!

weixiuli commented Sep 8, 2025

Uh oh!

JkSelf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

majetideepak left a comment

Choose a reason for hiding this comment

Uh oh!

weixiuli commented Sep 10, 2025

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weixiuli commented Sep 15, 2025

Uh oh!

JkSelf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weixiuli commented Aug 14, 2025 •

edited

Loading

netlify bot commented Aug 14, 2025 •

edited

Loading

majetideepak commented Jan 27, 2026 •

edited

Loading