Enforce queuing memory limit on exchange clients in merge exchange #7410

bikramSingh91 · 2023-11-03T23:50:59Z

Currently, when the merge join creates ExchangeClients for each
source, they are subject to a 32MB queuing limit. However, this can
lead to high memory usage if all clients queue around 32MB. For
instance, we observed 300 clients consuming over 6GB of memory in one
case. To address this issue, we have introduced a new query config
"merge_exchange.max_buffer_size" that sets an upper bound on the total
memory that can be queued by these clients. This max limit is divided
equally among all clients with an upper and lower limit of 32MB and
1MB, respectively, per client. The default for this config is set to
128MB. It's important to note that this limit is enforced
approximately, not strictly.

netlify · 2023-11-03T23:51:06Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`7a8622b`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/654953d66305df00083cdf40

bikramSingh91 · 2023-11-03T23:54:29Z

Adding a unit test is proving to be tricky since we only use localExchangeClients within velox tests that do not allocate any memory. We can potentially implement a new Exchange client only for testing that emulates remote clients by redundantly copying the vectors it receives from local partitioned output. Will explore this option but open to suggestions.

facebook-github-bot · 2023-11-03T23:55:00Z

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

mbasmanova

Thanks.

mbasmanova · 2023-11-04T00:26:15Z

velox/docs/configs.rst

+   * - merge_exchange.max_buffer_size
+     - integer
+     - 128MB
+     - The aggregate buffer size (in Bytes) across all exchange clients generated by the merge exchange operator,


Bytes -> bytes

mbasmanova · 2023-11-04T00:30:54Z

typo in PR description:

Currently, when the merge join creates ExchangeClients

join -> exchange

xiaoxmeng

@bikramSingh91 nice change. Thanks!

xiaoxmeng · 2023-11-04T03:01:46Z

velox/exec/Merge.cpp

      } else {
        noMoreSplits_ = true;
+        auto maxMergeExchangeBufferSize = operatorCtx_->driverCtx()


const auto maxMergeExchangeBufferSize

xiaoxmeng · 2023-11-04T03:01:56Z

velox/exec/Merge.cpp

+        auto maxMergeExchangeBufferSize = operatorCtx_->driverCtx()
+                                              ->queryConfig()
+                                              .maxMergeExchangeBufferSize();
+        auto maxQueuedBytesPerSource = std::min<int64_t>(


xiaoxmeng · 2023-11-04T03:05:12Z

velox/exec/Merge.cpp

+                maxMergeExchangeBufferSize / remoteSourceTaskIds_.size(),
+                MergeSource::kMaxQueuedBytesLowerLimit),
+            MergeSource::kMaxQueuedBytesUpperLimit);
+        uint32_t currentSourceId = 0;


nit

for (uint32_t remoteSourceIndex = 0; remoteSourceIndex < remoteSourceTaskIds_.size(); ++remoteSourceIndex) { auto* pool = operatorCtx_->task()->addMergeSourcePool(..., emoteSourceIndex); sources_.emplace_back(..., remoteSourceTaskIds_[remoteSourceIndex], ... }

xiaoxmeng · 2023-11-04T03:05:38Z

velox/exec/MergeSource.cpp

@@ -120,13 +120,14 @@ class MergeExchangeSource : public MergeSource {
      MergeExchange* mergeExchange,
      const std::string& taskId,
      int destination,
-      memory::MemoryPool* FOLLY_NONNULL pool)
+      memory::MemoryPool* FOLLY_NONNULL pool,


NYC: can just remove FOLLY_NONNULL? Thanks!

xiaoxmeng · 2023-11-04T03:06:22Z

velox/exec/MergeSource.cpp

@@ -207,9 +208,10 @@ std::shared_ptr<MergeSource> MergeSource::createMergeExchangeSource(
    MergeExchange* mergeExchange,
    const std::string& taskId,
    int destination,
-    memory::MemoryPool* pool) {
+    memory::MemoryPool* pool,
+    int64_t maxQueuedBytes) {


nit: maybe keep pool the last parameter? thanks!

xiaoxmeng · 2023-11-04T03:06:46Z

velox/exec/MergeSource.h

@@ -23,6 +23,9 @@ class MergeExchange;

 class MergeSource {
 public:
+  static constexpr int32_t kMaxQueuedBytesUpperLimit = 32 << 20; // 32 MB.


Does these need to be public? If not, let's move to private section? thanks!

These are used by MergeExchange::addMergeSources() so need to be public. The other alternative is that we keep them private and handle the enforcement of upper and lower limit within MergeSource::createMergeExchangeSource(). This would abstract away the enforcement logic from the caller (MergeExchange) but this means the client loses explicit control. Either way is fine with me since we dont have other generic use cases yet for either alternative.
Please let me know if you want me to change the current implementation either way. Thanks

xiaoxmeng · 2023-11-04T03:06:54Z

velox/exec/MergeSource.h

@@ -40,7 +43,8 @@ class MergeSource {
      MergeExchange* mergeExchange,
      const std::string& taskId,
      int destination,
-      memory::MemoryPool* FOLLY_NONNULL pool);
+      memory::MemoryPool* FOLLY_NONNULL pool,


…acebookincubator#7410) Summary: Currently, when the merge exchange creates ExchangeClients for each source, they are subject to a 32MB queuing limit. However, this can lead to high memory usage if all clients queue around 32MB. For instance, we observed 300 clients consuming over 6GB of memory in one case. To address this issue, we have introduced a new query config "merge_exchange.max_buffer_size" that sets an upper bound on the total memory that can be queued by these clients. This max limit is divided equally among all clients with an upper and lower limit of 32MB and 1MB, respectively, per client. The default for this config is set to 128MB. It's important to note that this limit is enforced approximately, not strictly. Reviewed By: xiaoxmeng, mbasmanova Differential Revision: D50996298 Pulled By: bikramSingh91

facebook-github-bot · 2023-11-06T19:45:27Z

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

…acebookincubator#7410) Summary: Currently, when the merge exchange creates ExchangeClients for each source, they are subject to a 32MB queuing limit. However, this can lead to high memory usage if all clients queue around 32MB. For instance, we observed 300 clients consuming over 6GB of memory in one case. To address this issue, we have introduced a new query config "merge_exchange.max_buffer_size" that sets an upper bound on the total memory that can be queued by these clients. This max limit is divided equally among all clients with an upper and lower limit of 32MB and 1MB, respectively, per client. The default for this config is set to 128MB. It's important to note that this limit is enforced approximately, not strictly. Reviewed By: xiaoxmeng, mbasmanova Differential Revision: D50996298 Pulled By: bikramSingh91

facebook-github-bot · 2023-11-06T19:57:40Z

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

…acebookincubator#7410) Summary: Currently, when the merge exchange creates ExchangeClients for each source, they are subject to a 32MB queuing limit. However, this can lead to high memory usage if all clients queue around 32MB. For instance, we observed 300 clients consuming over 6GB of memory in one case. To address this issue, we have introduced a new query config "merge_exchange.max_buffer_size" that sets an upper bound on the total memory that can be queued by these clients. This max limit is divided equally among all clients with an upper and lower limit of 32MB and 1MB, respectively, per client. The default for this config is set to 128MB. It's important to note that this limit is enforced approximately, not strictly. Reviewed By: xiaoxmeng, mbasmanova Differential Revision: D50996298 Pulled By: bikramSingh91

facebook-github-bot · 2023-11-06T21:00:13Z

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

facebook-github-bot · 2023-11-07T00:21:26Z

@bikramSingh91 merged this pull request in 9bb4de4.

conbench-facebook · 2023-11-07T00:56:52Z

Conbench analyzed the 1 benchmark run on commit 9bb4de4a.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

bikramSingh91 requested review from mbasmanova and xiaoxmeng November 3, 2023 23:50

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 Nov 3, 2023

mbasmanova approved these changes Nov 4, 2023

View reviewed changes

xiaoxmeng approved these changes Nov 4, 2023

View reviewed changes

bikramSingh91 force-pushed the mergeExchangeMemLimit branch from cb0a584 to 1658217 Compare November 6, 2023 19:45

facebook-github-bot added the fb-exported label Nov 6, 2023

bikramSingh91 force-pushed the mergeExchangeMemLimit branch from 1658217 to 310d26b Compare November 6, 2023 19:57

bikramSingh91 force-pushed the mergeExchangeMemLimit branch from 310d26b to 7a8622b Compare November 6, 2023 21:00

facebook-github-bot closed this in 9bb4de4 Nov 7, 2023

facebook-github-bot added the Merged label Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce queuing memory limit on exchange clients in merge exchange #7410

Enforce queuing memory limit on exchange clients in merge exchange #7410

bikramSingh91 commented Nov 3, 2023

netlify bot commented Nov 3, 2023 •

edited

bikramSingh91 commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

mbasmanova left a comment

mbasmanova Nov 4, 2023

mbasmanova commented Nov 4, 2023

xiaoxmeng left a comment

xiaoxmeng Nov 4, 2023

xiaoxmeng Nov 4, 2023

xiaoxmeng Nov 4, 2023

xiaoxmeng Nov 4, 2023

xiaoxmeng Nov 4, 2023

xiaoxmeng Nov 4, 2023

bikramSingh91 Nov 6, 2023

xiaoxmeng Nov 4, 2023

facebook-github-bot commented Nov 6, 2023

facebook-github-bot commented Nov 6, 2023

facebook-github-bot commented Nov 6, 2023

facebook-github-bot commented Nov 7, 2023

conbench-facebook bot commented Nov 7, 2023

Enforce queuing memory limit on exchange clients in merge exchange #7410

Enforce queuing memory limit on exchange clients in merge exchange #7410

Conversation

bikramSingh91 commented Nov 3, 2023

netlify bot commented Nov 3, 2023 • edited

✅ Deploy Preview for meta-velox canceled.

bikramSingh91 commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova commented Nov 4, 2023

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

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 Nov 6, 2023

facebook-github-bot commented Nov 6, 2023

facebook-github-bot commented Nov 6, 2023

facebook-github-bot commented Nov 7, 2023

conbench-facebook bot commented Nov 7, 2023

netlify bot commented Nov 3, 2023 •

edited