[native] Default task.max-drivers-per-task to hardware concurrency #24642

majetideepak · 2025-02-27T15:57:16Z

Description

The doc says task.max-drivers-per-task defaults to CPU count.
https://prestodb.io/docs/current/presto_cpp/properties.html#task-max-drivers-per-task
But it defaults to 16 in the code.
This config must default to the thread concurrency count for better user experience.

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Prestissimo (Native Execution)
* Fix task.max-drivers-per-task to use thread concurrency of the host

steveburnett

LGTM! (docs)

Pull branch, local doc build, looks good. Thanks!

czentgr

Thanks for this!

czentgr · 2025-02-27T16:58:57Z

presto-native-execution/presto_cpp/main/common/Configs.cpp

@@ -76,21 +76,6 @@ std::string ConfigBase::capacityPropertyAsBytesString(
      velox::config::CapacityUnit::BYTE));
 }

-bool ConfigBase::registerProperty(


Thanks, looks like dead code.

czentgr · 2025-02-27T17:00:41Z

presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp

-  ASSERT_EQ(config.maxDriversPerTask(), 16);
+  // std::thread::hardware_concurrency() value depends on the host.
+  // Most hardware should have atleast 8 threads.
+  ASSERT_GE(config.maxDriversPerTask(), 8);


Don't you want to not use 8 hard coded here but also read it from std::thread::hardware_concurrency() and also to account for when this is 0?
We've run in all kinds of build envs so this could be a failure point.

Good point!

majetideepak · 2025-02-27T18:36:45Z

presto-native-execution/presto_cpp/main/common/Configs.cpp

-        "{} must be one of 'LEAF', 'INTERMEDIATE', or 'DEFAULT'",
-        kPoolType);
-    return value;
+  static const std::unordered_set<std::string> kPoolTypes = {


format changes

czentgr

Thanks, looks good.

aditi-pandit

Thanks @majetideepak. Small nit.

aditi-pandit · 2025-02-27T18:57:46Z

presto-native-execution/presto_cpp/main/common/Configs.cpp

+  auto numThreads = std::thread::hardware_concurrency();
+  // The spec says std::thread::hardware_concurrency() might return 0.
+  if (numThreads <= 0) {
+    numThreads = 16;


Nit : Add static constant for this number.

This function is in an anonymous namespace. I don't think there is any advantage of using a constant here. All the default config values use literals for example.

@majetideepak : All of the SystemConfig property defaults are either in the registeredProps_ map (where static constant isn't needed) or they have individual SystemConfig functions to retrieve the properties if there is some logic like this which could use a default value. They aren't in anonymous namespace functions.

We should be consistent with that in general.

Using an anonymous namespace is equivalent to using static. https://stackoverflow.com/questions/4422507/how-are-unnamed-namespaces-superior-to-the-static-keyword
I can add a constant.

@majetideepak : Thanks for that article. Agree that we should reduce our use of static constants and functions as much as possible for external linkage.

Made the review comment largely because of consistency with the rest of this class.

@aditi-pandit I ended up removing the constant. I added a check instead.

aditi-pandit

Thanks @majetideepak

majetideepak · 2025-02-27T22:26:48Z

@steveburnett Can you approve this again? Thanks.

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build.

zacw7 · 2025-02-28T17:40:08Z

presto-native-execution/presto_cpp/main/common/Configs.h

-  /// Registers an extra property in the config.
-  /// Returns true if succeeded, false if failed (due to the property already
-  /// registered).
-  bool registerProperty(


Hey @majetideepak , why do we remove this? We still have usage of this function inside meta.

In OSS, its unused. We should probably add some tests or at least leave a comment about its usage.

majetideepak requested review from steveburnett, elharo and a team as code owners February 27, 2025 15:57

majetideepak requested a review from presto-oss February 27, 2025 15:57

prestodb-ci added the from:IBM PR from IBM label Feb 27, 2025

prestodb-ci requested review from a team, ScrapCodes and imsayari404 and removed request for a team February 27, 2025 15:57

majetideepak force-pushed the fix-default-workers branch from 7faa001 to 765ca84 Compare February 27, 2025 15:59

majetideepak requested review from amitkdutta and aditi-pandit and removed request for ScrapCodes and imsayari404 February 27, 2025 16:00

steveburnett previously approved these changes Feb 27, 2025

View reviewed changes

czentgr reviewed Feb 27, 2025

View reviewed changes

majetideepak dismissed steveburnett’s stale review via 5f3ba01 February 27, 2025 18:09

majetideepak force-pushed the fix-default-workers branch 2 times, most recently from 5f3ba01 to 40a7405 Compare February 27, 2025 18:19

majetideepak requested review from czentgr and steveburnett February 27, 2025 18:19

majetideepak commented Feb 27, 2025

View reviewed changes

czentgr previously approved these changes Feb 27, 2025

View reviewed changes

aditi-pandit reviewed Feb 27, 2025

View reviewed changes

majetideepak dismissed czentgr’s stale review via 989f107 February 27, 2025 20:38

majetideepak force-pushed the fix-default-workers branch 2 times, most recently from 989f107 to 6dc3a20 Compare February 27, 2025 20:38

[native] Default task.max-drivers-per-task to hardware concurrency

6dc3a20

majetideepak requested a review from czentgr February 27, 2025 20:40

majetideepak requested a review from aditi-pandit February 27, 2025 20:40

aditi-pandit approved these changes Feb 27, 2025

View reviewed changes

steveburnett approved these changes Feb 27, 2025

View reviewed changes

majetideepak merged commit f6c9750 into prestodb:master Feb 28, 2025
61 checks passed

majetideepak deleted the fix-default-workers branch February 28, 2025 17:41

zacw7 reviewed Mar 1, 2025

View reviewed changes

This was referenced Mar 10, 2025

Add release notes for 0.292 unix280/presto#5

Closed

Add release notes for 0.292 unix280/presto#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native] Default task.max-drivers-per-task to hardware concurrency #24642

[native] Default task.max-drivers-per-task to hardware concurrency #24642

majetideepak commented Feb 27, 2025 •

edited

Loading

steveburnett left a comment

czentgr left a comment

czentgr Feb 27, 2025

majetideepak Feb 27, 2025

czentgr Feb 27, 2025

majetideepak Feb 27, 2025

majetideepak Feb 27, 2025

czentgr left a comment

aditi-pandit left a comment

aditi-pandit Feb 27, 2025

majetideepak Feb 27, 2025

aditi-pandit Feb 27, 2025

majetideepak Feb 27, 2025

aditi-pandit Feb 27, 2025

majetideepak Feb 27, 2025

aditi-pandit left a comment

majetideepak commented Feb 27, 2025

steveburnett left a comment

zacw7 Feb 28, 2025

amitkdutta Mar 1, 2025

[native] Default task.max-drivers-per-task to hardware concurrency #24642

[native] Default task.max-drivers-per-task to hardware concurrency #24642

Conversation

majetideepak commented Feb 27, 2025 • edited Loading

Description

Contributor checklist

Release Notes

steveburnett left a comment

Choose a reason for hiding this comment

czentgr 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

czentgr left a comment

Choose a reason for hiding this comment

aditi-pandit 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

aditi-pandit left a comment

Choose a reason for hiding this comment

majetideepak commented Feb 27, 2025

steveburnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Feb 27, 2025 •

edited

Loading