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

Author: majetideepak

presto-docs/src/main/sphinx/presto_cpp/properties.rst

@@ -129,9 +129,10 @@ The configuration properties of Presto C++ workers are described here, in alphab
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 * **Type:** ``integer``
-* **Default value:** ``number of hardware CPUs``
+* **Default value:** ``number of concurrent threads supported by the host``
 
-  Number of drivers to use per task. Defaults to hardware CPUs.
+  Number of drivers to use per task. Defaults to the number of concurrent
+  threads supported by the host if computable or 16.
 
 ``query.max-memory-per-node``
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

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

@@ -76,21 +76,6 @@ std::string ConfigBase::capacityPropertyAsBytesString(
       velox::config::CapacityUnit::BYTE));
 }
 
-bool ConfigBase::registerProperty(
-    const std::string& propertyName,
-    const folly::Optional<std::string>& defaultValue) {
-  if (registeredProps_.count(propertyName) != 0) {
-    PRESTO_STARTUP_LOG(WARNING)
-        << "Property '" << propertyName
-        << "' is already registered with default value '"
-        << registeredProps_[propertyName].value_or("<none>") << "'.";
-    return false;
-  }
-
-  registeredProps_[propertyName] = defaultValue;
-  return true;
-}
-
 folly::Optional<std::string> ConfigBase::setValue(
     const std::string& propertyName,
     const std::string& value) {
@@ -138,7 +123,7 @@ SystemConfig::SystemConfig() {
           BOOL_PROP(kHttpServerReusePort, false),
           BOOL_PROP(kHttpServerBindToNodeInternalAddressOnlyEnabled, false),
           NONE_PROP(kDiscoveryUri),
-          NUM_PROP(kMaxDriversPerTask, 16),
+          NONE_PROP(kMaxDriversPerTask),
           NONE_PROP(kTaskWriterCount),
           NONE_PROP(kTaskPartitionedWriterCount),
           NUM_PROP(kConcurrentLifespansPerTask, 1),
@@ -292,14 +277,16 @@ std::string SystemConfig::prestoVersion() const {
 }
 
 std::string SystemConfig::poolType() const {
-    static const std::unordered_set<std::string> kPoolTypes = {"LEAF", "INTERMEDIATE", "DEFAULT"};
-    static constexpr std::string_view kPoolTypeDefault = "DEFAULT";
-    auto value = optionalProperty<std::string>(kPoolType).value_or(std::string(kPoolTypeDefault));
-    VELOX_USER_CHECK(
-        kPoolTypes.count(value),
-        "{} must be one of 'LEAF', 'INTERMEDIATE', or 'DEFAULT'",
-        kPoolType);
-    return value;
+  static const std::unordered_set<std::string> kPoolTypes = {
+      "LEAF", "INTERMEDIATE", "DEFAULT"};
+  static constexpr std::string_view kPoolTypeDefault = "DEFAULT";
+  auto value = optionalProperty<std::string>(kPoolType).value_or(
+      std::string(kPoolTypeDefault));
+  VELOX_USER_CHECK(
+      kPoolTypes.count(value),
+      "{} must be one of 'LEAF', 'INTERMEDIATE', or 'DEFAULT'",
+      kPoolType);
+  return value;
 }
 
 bool SystemConfig::mutableConfig() const {
@@ -355,7 +342,17 @@ std::string SystemConfig::remoteFunctionServerSerde() const {
 }
 
 int32_t SystemConfig::maxDriversPerTask() const {
-  return optionalProperty<int32_t>(kMaxDriversPerTask).value();
+  auto val = optionalProperty<int32_t>(kMaxDriversPerTask);
+  if (val.has_value()) {
+    return val.value();
+  }
+  auto numThreads = std::thread::hardware_concurrency();
+  // The spec says std::thread::hardware_concurrency() might return 0.
+  if (numThreads > 0) {
+    return numThreads;
+  } else {
+    return 16;
+  }
 }
 
 folly::Optional<int32_t> SystemConfig::taskWriterCount() const {

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

@@ -42,13 +42,6 @@ class ConfigBase {
     config_ = std::move(config);
   }
 
-  /// Registers an extra property in the config.
-  /// Returns true if succeeded, false if failed (due to the property already
-  /// registered).
-  bool registerProperty(
-      const std::string& propertyName,
-      const folly::Optional<std::string>& defaultValue = {});
-
   /// Adds or replaces value at the given key. Can be used by debugging or
   /// testing code.
   /// Returns previous value if there was any.

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

@@ -11,8 +11,8 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include <filesystem>
 #include <gtest/gtest.h>
+#include <filesystem>
 #include <unordered_set>
 #include "presto_cpp/main/common/ConfigReader.h"
 #include "presto_cpp/main/common/Configs.h"
@@ -217,7 +217,9 @@ TEST_F(ConfigTest, optionalNodeConfigs) {
 TEST_F(ConfigTest, optionalSystemConfigsWithDefault) {
   SystemConfig config;
   init(config, {});
-  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);
   init(config, {{std::string(SystemConfig::kMaxDriversPerTask), "1024"}});
   ASSERT_EQ(config.maxDriversPerTask(), 1024);
 }