[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.
 
 ``query.max-memory-per-node``
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

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

@@ -37,6 +37,15 @@ std::string bool2String(bool value) {
   return value ? "true" : "false";
 }
 
+int getThreadCount() {
+  auto numThreads = std::thread::hardware_concurrency();
+  // The spec says std::thread::hardware_concurrency() might return 0.
+  // But we depend on std::thread::hardware_concurrency() to create executors.
+  // Add a check to ensure it is > 0.
+  VELOX_CHECK_GT(numThreads, 0);
+  return numThreads;
+}
+
 #define STR_PROP(_key_, _val_) \
   { std::string(_key_), std::string(_val_) }
 #define NUM_PROP(_key_, _val_) \
@@ -76,21 +85,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 +132,7 @@ SystemConfig::SystemConfig() {
           BOOL_PROP(kHttpServerReusePort, false),
           BOOL_PROP(kHttpServerBindToNodeInternalAddressOnlyEnabled, false),
           NONE_PROP(kDiscoveryUri),
-          NUM_PROP(kMaxDriversPerTask, 16),
+          NUM_PROP(kMaxDriversPerTask, getThreadCount()),
           NONE_PROP(kTaskWriterCount),
           NONE_PROP(kTaskPartitionedWriterCount),
           NUM_PROP(kConcurrentLifespansPerTask, 1),
@@ -292,14 +286,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 {

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,7 @@ TEST_F(ConfigTest, optionalNodeConfigs) {
 TEST_F(ConfigTest, optionalSystemConfigsWithDefault) {
   SystemConfig config;
   init(config, {});
-  ASSERT_EQ(config.maxDriversPerTask(), 16);
+  ASSERT_EQ(config.maxDriversPerTask(), std::thread::hardware_concurrency());
   init(config, {{std::string(SystemConfig::kMaxDriversPerTask), "1024"}});
   ASSERT_EQ(config.maxDriversPerTask(), 1024);
 }

presto-native-execution/presto_cpp/main/tests/ServerOperationTest.cpp

@@ -240,7 +240,8 @@ TEST_F(ServerOperationTest, systemConfigEndpoint) {
       {.target = ServerOperation::Target::kSystemConfig,
        .action = ServerOperation::Action::kGetProperty},
       &httpMessage);
-  EXPECT_EQ(getPropertyResponse, "16\n");
+  EXPECT_EQ(
+      std::stoi(getPropertyResponse), std::thread::hardware_concurrency());
 }
 
 } // namespace facebook::presto