misc: Improve OutputBufferManager initialization #11350

yingsu00 · 2024-10-25T06:45:33Z

Starting from C++11, the C++ standard guarantees that the initialization
of function-local static variables is thread-safe. This is better than
using a global mutex, especially for subsequent getInstance() calls. This
is because the overhead of using a static local variable only needs to do
a simple check to see if the variable has already been initialized, while
for the global mutex case, all getInstance() calls need to aquire this
lock exclusively.

netlify · 2024-10-25T06:45:51Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`432dc90`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/675213e7f6e2970007ab12ff

Yuhta · 2024-10-28T15:26:55Z

velox/exec/OutputBufferManager.cpp

-  return instance_;
+std::weak_ptr<OutputBufferManager> OutputBufferManager::getInstance(
+    const Options* optionsPtr) {
+  auto options = optionsPtr ? *optionsPtr : Options();


We lose some check to make sure no new option is passed in if the instance is already created. On the other hand, it seems OutputBufferManager::initialize is not used anywhere. Maybe just drop the option parameter altogether?

Also I don't see why it needs to be a weak_ptr. Should just return const shared_ptr<OutputBufferManager>&.

Thanks @Yuhta I have updated the code to

std::shared_ptr<OutputBufferManager> OutputBufferManager::getInstance() { static std::shared_ptr<OutputBufferManager> instance = std::make_shared<OutputBufferManager>(Options()); return instance; }

Appreciate your review again!

Yuhta · 2024-10-30T18:16:42Z

velox/exec/OutputBufferManager.cpp

-    instance_ = std::make_shared<OutputBufferManager>(Options());
-  }
-  return instance_;
+std::shared_ptr<OutputBufferManager> OutputBufferManager::getInstance() {


We can return const std::shared_ptr<OutputBufferManager>& to avoid a copy and atomic add (thus cache invalidation).

facebook-github-bot · 2024-11-04T15:07:29Z

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

mbasmanova · 2024-11-04T15:13:06Z

@yingsu00 CI is red. Please, take a look. We need a green CI to merge.

yingsu00 · 2024-11-06T05:57:10Z

@yingsu00 CI is red. Please, take a look. We need a green CI to merge.

CI is green now. Thanks for porting it.

ManManson · 2024-11-06T10:20:02Z

Thanks for working on this!

facebook-github-bot · 2024-11-06T16:07:45Z

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

mbasmanova · 2024-11-06T17:20:20Z

@yingsu00 Seeing failures in Prestissimo. If this change is not backwards compatible it needs to be fixed.

fbcode/github/presto-trunk/presto-native-execution/presto_cpp/main/PeriodicTaskManager.cpp:239:55: error: no member named 'lock' in 'std::shared_ptr<facebook::velox::exec::OutputBufferManager>'
  239 |       velox::exec::OutputBufferManager::getInstance().lock()->numBuffers());
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
buck-out/v2/gen/fbcode/4ac7a641bc650342/velox/common/base/__velox_stats_reporter__/buck-headers/velox/common/base/StatsReporter.h:208:43: note: expanded from macro 'RECORD_METRIC_VALUE'
  208 |         reporter->addMetricValue((key), ##__VA_ARGS__);        \
      |                                           ^~~~~~~~~~~
1 error generated.

yingsu00 · 2024-11-07T01:08:00Z

@yingsu00 Seeing failures in Prestissimo. If this change is not backwards compatible it needs to be fixed.

fbcode/github/presto-trunk/presto-native-execution/presto_cpp/main/PeriodicTaskManager.cpp:239:55: error: no member named 'lock' in 'std::shared_ptr<facebook::velox::exec::OutputBufferManager>'
  239 |       velox::exec::OutputBufferManager::getInstance().lock()->numBuffers());
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
buck-out/v2/gen/fbcode/4ac7a641bc650342/velox/common/base/__velox_stats_reporter__/buck-headers/velox/common/base/StatsReporter.h:208:43: note: expanded from macro 'RECORD_METRIC_VALUE'
  208 |         reporter->addMetricValue((key), ##__VA_ARGS__);        \
      |                                           ^~~~~~~~~~~
1 error generated.

I'll send a PR to Presto repo to fix this.

yingsu00 · 2024-11-07T07:48:21Z

@yingsu00 Seeing failures in Prestissimo. If this change is not backwards compatible it needs to be fixed.

fbcode/github/presto-trunk/presto-native-execution/presto_cpp/main/PeriodicTaskManager.cpp:239:55: error: no member named 'lock' in 'std::shared_ptr<facebook::velox::exec::OutputBufferManager>'
  239 |       velox::exec::OutputBufferManager::getInstance().lock()->numBuffers());
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
buck-out/v2/gen/fbcode/4ac7a641bc650342/velox/common/base/__velox_stats_reporter__/buck-headers/velox/common/base/StatsReporter.h:208:43: note: expanded from macro 'RECORD_METRIC_VALUE'
  208 |         reporter->addMetricValue((key), ##__VA_ARGS__);        \
      |                                           ^~~~~~~~~~~
1 error generated.

I'll send a PR to Presto repo to fix this.

Draft Presto PR: prestodb/presto#23970. How do you want to proceed?
Once this Velox PR is merged, I can add an advance Velox commit in prestodb/presto#23970. The "advance Velox" will need the hash of this commit.

mbasmanova · 2024-11-07T12:20:16Z

@yingsu00 We cannot merge Velox PR that breaks Prestissimo. The changes need to be made in backward-compatible manner.

mbasmanova · 2024-11-07T12:24:27Z

@yingsu00 Some examples: #5953 #6459

yingsu00 · 2024-11-07T23:51:22Z

@yingsu00 Some examples: #5953 #6459

@mbasmanova Thanks for the examples, but could you elaborate a bit how to follow these examples? Since OutputbufferManager::getInstance() now returns shared_ptr, all callsites, including the ones in Presto, needs to remove the lock() from OutputbufferManager::getInstance().lock(). Did you mean you wanted to keep it returning weak_ptr? Returning weak_ptr is not quite appropriate here, as @Yuhta suggested. It makes sense to keep any class fields to this object a weak_ptr, e.g. in PartitionedOutput const std::weak_ptr<exec::OutputBufferManager> bufferManager_;, but it's better to return a shared_ptr for OutputbufferManager::getInstance().

Also I don't quite understand why merging this PR would break Presto given that Presto needs to advance Velox version to pick up this change. Presto won't break unless someone else advances Velox for Presto without merging prestodb/presto#23970 together. To avoid that situation, I will add "Advance Velox" change to prestodb/presto#23970 once I get the hash after this change is merged to Velox. Do you think if it works this way? Or do you have other suggestions? Appreciate it if you could elaborate more. Thanks!

yingsu00 · 2024-11-13T01:38:26Z

@ManManson @mbasmanova @Yuhta Can anyone provide some advice how to proceed? Thank you!

ManManson · 2024-11-13T14:30:59Z

The most straightforward way would be to split the work into several pieces as follows:

Introduce std::shared_ptr<...> getInstanceStrong() and convert uses that are strictly confined to Velox internals, but leave public APIs (which are used by Prestissimo) as is.
Upgrade Velox version in Presto C++ and convert all references to OutputBufferManager to use getInstanceStrong(). This ensures that we don't have references to the old method signature anymore.
Make old getInstance() to also return std::shared_ptr<...> (so getInstance() and getInstanceStrong() become the same thing).
Upgrade Velox version in Presto C++ once again and convert all uses of getInstanceStrong() to getInstance().
Now we are free to finally drop the getInstanceStrong() method.

yingsu00 · 2024-11-13T22:42:20Z

The most straightforward way would be to split the work into several pieces as follows:

Introduce std::shared_ptr<...> getInstanceStrong() and convert uses that are strictly confined to Velox internals, but leave public APIs (which are used by Prestissimo) as is.

Upgrade Velox version in Presto C++ and convert all references to OutputBufferManager to use getInstanceStrong(). This ensures that we don't have references to the old method signature anymore.

Make old getInstance() to also return std::shared_ptr<...> (so getInstance() and getInstanceStrong() become the same thing).

Upgrade Velox version in Presto C++ once again and convert all uses of getInstanceStrong() to getInstance().

Now we are free to finally drop the getInstanceStrong() method.

Cool! Thanks for the idea @ManManson. I have updated this PR to the state of 1) as you suggested. Please re-review.

yingsu00 · 2024-11-15T06:12:34Z

@ManManson @Yuhta Will you folks be able to review this PR again?

ManManson · 2024-11-15T17:26:36Z

velox/exec/OutputBufferManager.cpp

+const std::shared_ptr<OutputBufferManager>&
+OutputBufferManager::getInstanceStrong() {
+  static const std::shared_ptr<OutputBufferManager> instance =
+      std::make_shared<OutputBufferManager>(Options());
+  return instance;
+}


The OutputBufferManager::getInstance() should be implemented in terms of getInstanceStrong(). Something like this:

std::weak_ptr<OutputBufferManager> OutputBufferManager::getInstance() { return getInstanceStrong(); }

Otherwise it will break any existing consumer of Velox since getInstance() and getInstanceStrong() would return different instances.

ManManson

LGTM

yingsu00 · 2024-11-19T03:15:33Z

Thank you @ManManson ! @Yuhta @mbasmanova Do you have other comments? If not I'm marking this PR to be "ready-to-merge" again. Once this one is merged I'll send follow up PRs as @ManManson suggested.

velox/exec/OutputBufferManager.h

facebook-github-bot · 2024-12-04T12:01:33Z

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

mbasmanova · 2024-12-04T13:56:49Z

@yingsu00 Ying, would you rebase so we can try landing this PR?

facebook-github-bot · 2024-12-04T21:51:39Z

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

mbasmanova · 2024-12-04T23:52:44Z

@yingsu00 Thank you for rebasing. Seeing CI failures. Would you take a look?

yingsu00 · 2024-12-05T01:00:33Z

Details

@mbasmanova Thanks for importing it! The test failures seems to be caused by #11737 which was exported and not reviewed. #11743 is supposed to fix it. We can wait it to be merged first then I'll rebase again.

Starting from C++11, the C++ standard guarantees that the initialization of function-local static variables is thread-safe. This is better than using a global mutex, especially for subsequent getInstance() calls. This is because the overhead of using a static local variable only needs to do a simple check to see if the variable has already been initialized, while for the global mutex case, all getInstance() calls need to aquire this lock exclusively.

facebook-github-bot · 2024-12-05T21:00:50Z

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

mbasmanova · 2024-12-05T23:41:09Z

CI is still red.

yingsu00 · 2024-12-06T22:37:14Z

hi @mbasmanova All tests pass now except the Facebook internal ones:

Facebook Internal - Builds & Tests:
3 warning_suppressed, 3 warning, 6 warning_preexisting, 94 warning_nosignal, 893 advice_no_signal
3 warning_suppressed, 3 warning, 6 warning_preexisting, 94 warning_nosignal, 893 advice_no_signal internally at Meta.
warning:
infer-checkers-fbcode-diff-analysis-with-buck

I'm unable to view them though. Would it be possible to show me what the warnings are? I can fix them once I know what's wrong. Thank you!

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 Oct 25, 2024

yingsu00 marked this pull request as ready for review October 26, 2024 02:17

yingsu00 requested review from assignUser and majetideepak as code owners October 26, 2024 02:17

yingsu00 requested review from kgpai and Yuhta October 26, 2024 02:20

yingsu00 force-pushed the static branch from 06abad1 to 5a1b5a1 Compare October 26, 2024 02:21

Yuhta reviewed Oct 28, 2024

View reviewed changes

yingsu00 force-pushed the static branch from 5a1b5a1 to 6e7445d Compare October 29, 2024 00:40

Yuhta reviewed Oct 30, 2024

View reviewed changes

yingsu00 force-pushed the static branch 3 times, most recently from 189aea0 to 67d13ea Compare November 2, 2024 00:33

Yuhta approved these changes Nov 4, 2024

View reviewed changes

Yuhta added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 4, 2024

yingsu00 force-pushed the static branch from 67d13ea to 74978c7 Compare November 6, 2024 03:25

ManManson approved these changes Nov 6, 2024

View reviewed changes

yingsu00 mentioned this pull request Nov 7, 2024

Improve OutputBufferManager getInstance prestodb/presto#23970

Draft

6 tasks

mbasmanova removed the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 7, 2024

yingsu00 force-pushed the static branch from 74978c7 to 61ed96e Compare November 13, 2024 00:37

yingsu00 force-pushed the static branch from 61ed96e to 2998211 Compare November 13, 2024 22:40

yingsu00 requested a review from rui-mo November 15, 2024 03:33

ManManson suggested changes Nov 15, 2024

View reviewed changes

yingsu00 force-pushed the static branch from 2998211 to 66ee6e1 Compare November 16, 2024 01:51

yingsu00 changed the title ~~Improve OutputBufferManager initialization~~ misc: Improve OutputBufferManager initialization Nov 16, 2024

yingsu00 force-pushed the static branch from 66ee6e1 to 1c5c957 Compare November 16, 2024 03:18

ManManson approved these changes Nov 16, 2024

View reviewed changes

yingsu00 added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 19, 2024

yingsu00 force-pushed the static branch from 1c5c957 to de4400c Compare December 4, 2024 00:55

xiaoxmeng approved these changes Dec 4, 2024

View reviewed changes

velox/exec/OutputBufferManager.h Outdated Show resolved Hide resolved

yingsu00 force-pushed the static branch from de4400c to fdfa1fd Compare December 4, 2024 21:37

yingsu00 force-pushed the static branch from fdfa1fd to 432dc90 Compare December 5, 2024 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: Improve OutputBufferManager initialization #11350

misc: Improve OutputBufferManager initialization #11350

yingsu00 commented Oct 25, 2024

netlify bot commented Oct 25, 2024 •

edited

Loading

Yuhta Oct 28, 2024 •

edited

Loading

yingsu00 Oct 29, 2024

Yuhta Oct 30, 2024

facebook-github-bot commented Nov 4, 2024

mbasmanova commented Nov 4, 2024

yingsu00 commented Nov 6, 2024

ManManson commented Nov 6, 2024

facebook-github-bot commented Nov 6, 2024

mbasmanova commented Nov 6, 2024

yingsu00 commented Nov 7, 2024

yingsu00 commented Nov 7, 2024

mbasmanova commented Nov 7, 2024

mbasmanova commented Nov 7, 2024

yingsu00 commented Nov 7, 2024

yingsu00 commented Nov 13, 2024

ManManson commented Nov 13, 2024

yingsu00 commented Nov 13, 2024

yingsu00 commented Nov 15, 2024

ManManson Nov 15, 2024

ManManson left a comment

yingsu00 commented Nov 19, 2024

facebook-github-bot commented Dec 4, 2024

mbasmanova commented Dec 4, 2024

facebook-github-bot commented Dec 4, 2024

mbasmanova commented Dec 4, 2024

yingsu00 commented Dec 5, 2024 •

edited

Loading

facebook-github-bot commented Dec 5, 2024

mbasmanova commented Dec 5, 2024

yingsu00 commented Dec 6, 2024

misc: Improve OutputBufferManager initialization #11350

Are you sure you want to change the base?

misc: Improve OutputBufferManager initialization #11350

Conversation

yingsu00 commented Oct 25, 2024

netlify bot commented Oct 25, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

Yuhta Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

yingsu00 Oct 29, 2024

Choose a reason for hiding this comment

Yuhta Oct 30, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Nov 4, 2024

mbasmanova commented Nov 4, 2024

yingsu00 commented Nov 6, 2024

ManManson commented Nov 6, 2024

facebook-github-bot commented Nov 6, 2024

mbasmanova commented Nov 6, 2024

yingsu00 commented Nov 7, 2024

yingsu00 commented Nov 7, 2024

mbasmanova commented Nov 7, 2024

mbasmanova commented Nov 7, 2024

yingsu00 commented Nov 7, 2024

yingsu00 commented Nov 13, 2024

ManManson commented Nov 13, 2024

yingsu00 commented Nov 13, 2024

yingsu00 commented Nov 15, 2024

ManManson Nov 15, 2024

Choose a reason for hiding this comment

ManManson left a comment

Choose a reason for hiding this comment

yingsu00 commented Nov 19, 2024

facebook-github-bot commented Dec 4, 2024

mbasmanova commented Dec 4, 2024

facebook-github-bot commented Dec 4, 2024

mbasmanova commented Dec 4, 2024

yingsu00 commented Dec 5, 2024 • edited Loading

facebook-github-bot commented Dec 5, 2024

mbasmanova commented Dec 5, 2024

yingsu00 commented Dec 6, 2024

netlify bot commented Oct 25, 2024 •

edited

Loading

Yuhta Oct 28, 2024 •

edited

Loading

yingsu00 commented Dec 5, 2024 •

edited

Loading