Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 28, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions presto-docs/src/main/sphinx/presto_cpp/properties.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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``
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
44 changes: 20 additions & 24 deletions presto-native-execution/presto_cpp/main/common/Configs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
// Check to ensure numThreads 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_) \
Expand Down Expand Up @@ -76,21 +85,6 @@ std::string ConfigBase::capacityPropertyAsBytesString(
velox::config::CapacityUnit::BYTE));
}

bool ConfigBase::registerProperty(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks like dead code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

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) {
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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 = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format changes

"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 {
Expand Down
7 changes: 0 additions & 7 deletions presto-native-execution/presto_cpp/main/common/Configs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.
Expand Down
4 changes: 2 additions & 2 deletions presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
}
Expand Down
3 changes: 2 additions & 1 deletion presto-native-execution/presto_cpp/main/tests/ServerOperationTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading