You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/01/05 16:52:16 UTC
[GitHub] [arrow] pitrou opened a new pull request #9105: ARROW-11009: [C++] Allow changing default memory pool with an environment variable
pitrou opened a new pull request #9105:
URL: https://github.com/apache/arrow/pull/9105
ARROW_DEFAULT_MEMORY_POOL can take the name of the desired memory pool backend
('jemalloc', 'mimalloc', 'system').
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #9105: ARROW-11009: [C++] Allow changing default memory pool with an environment variable
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9105:
URL: https://github.com/apache/arrow/pull/9105#discussion_r552053668
##########
File path: cpp/src/arrow/dataset/filter.cc
##########
@@ -704,28 +704,38 @@ using arrow::internal::JoinStrings;
std::string AndExpression::ToString() const {
return JoinStrings(
- {"(", left_operand_->ToString(), " and ", right_operand_->ToString(), ")"}, "");
+ std::vector<util::string_view>{"(", left_operand_->ToString(), " and ",
+ right_operand_->ToString(), ")"},
+ "");
Review comment:
@bkietz Do tell if this is too verbose for your taste (to be honest, I'm not sure why `JoinStrings` is used for simple concatenation here).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9105: ARROW-11009: [C++] Allow changing default memory pool with an environment variable
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9105:
URL: https://github.com/apache/arrow/pull/9105#discussion_r553562039
##########
File path: cpp/src/arrow/memory_pool.cc
##########
@@ -83,10 +87,94 @@ const char* je_arrow_malloc_conf =
#endif // ARROW_JEMALLOC
namespace arrow {
+namespace {
constexpr size_t kAlignment = 64;
-namespace {
+constexpr char kDefaultBackendEnvVar[] = "ARROW_DEFAULT_MEMORY_POOL";
+
+enum class MemoryPoolBackend : uint8_t { System, Jemalloc, Mimalloc };
+
+std::string BackendName(MemoryPoolBackend backend) {
+ switch (backend) {
+ case MemoryPoolBackend::System:
+ return "system";
+ case MemoryPoolBackend::Jemalloc:
+ return "jemalloc";
+ case MemoryPoolBackend::Mimalloc:
+ return "mimalloc";
+ default:
+ return ""; // XXX unreachable
+ }
+}
+
+std::vector<MemoryPoolBackend> SupportedBackends() {
+ std::vector<MemoryPoolBackend> backends = {
+#ifdef ARROW_JEMALLOC
+ MemoryPoolBackend::Jemalloc,
+#endif
+#ifdef ARROW_MIMALLOC
+ MemoryPoolBackend::Mimalloc,
+#endif
+ MemoryPoolBackend::System};
+ return backends;
+}
+
+const std::vector<MemoryPoolBackend> supported_backends = SupportedBackends();
+
+util::optional<MemoryPoolBackend> UserSelectedBackend() {
+ auto unsupported_backend = [](const std::string& name) {
+ std::vector<std::string> supported;
+ for (const auto backend : supported_backends) {
+ supported.push_back("'" + BackendName(backend) + "'");
+ }
+ ARROW_LOG(WARNING) << "Unsupported backend '" << name << "' specified in "
+ << kDefaultBackendEnvVar << " (supported backends are "
+ << internal::JoinStrings(supported, ", ") << ")";
+ };
+
+ auto maybe_name = internal::GetEnvVar(kDefaultBackendEnvVar);
+ if (!maybe_name.ok()) {
+ return {};
+ }
+ const auto name = *std::move(maybe_name);
+ if (name.empty()) {
+ return {};
+ } else if (name == "system") {
+ return MemoryPoolBackend::System;
+ } else if (name == "jemalloc") {
+#ifdef ARROW_JEMALLOC
+ return MemoryPoolBackend::Jemalloc;
+#else
+ unsupported_backend(name);
+#endif
+ } else if (name == "mimalloc") {
+#ifdef ARROW_MIMALLOC
+ return MemoryPoolBackend::Mimalloc;
+#else
+ unsupported_backend(name);
+#endif
+ } else {
+ unsupported_backend(name);
+ }
Review comment:
Nit: this would be more readable if you iterated through SupportedBackends and created a `map<name, backend>` then did lookup in that
##########
File path: cpp/src/arrow/memory_pool.cc
##########
@@ -404,14 +499,25 @@ Status mimalloc_memory_pool(MemoryPool** out) {
#endif
}
+static std::unique_ptr<MemoryPool> default_pool = MemoryPool::CreateDefault();
Review comment:
This seems to be unused. Did you intend to write
```c++
MemoryPool* default_memory_pool() { return default_pool.get(); }
```
?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #9105: ARROW-11009: [C++] Allow changing default memory pool with an environment variable
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9105:
URL: https://github.com/apache/arrow/pull/9105#discussion_r554926014
##########
File path: cpp/src/arrow/memory_pool.cc
##########
@@ -404,14 +499,25 @@ Status mimalloc_memory_pool(MemoryPool** out) {
#endif
}
+static std::unique_ptr<MemoryPool> default_pool = MemoryPool::CreateDefault();
Review comment:
Hmm, nice catch. No, it should point to one of the existing `system_pool` etc. (so that `default_pool` doesn't differ from e.g. `jemalloc_pool` when jemalloc is enabled)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz closed pull request #9105: ARROW-11009: [C++] Allow changing default memory pool with an environment variable
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #9105:
URL: https://github.com/apache/arrow/pull/9105
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9105: ARROW-11009: [C++] Allow changing default memory pool with an environment variable
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9105:
URL: https://github.com/apache/arrow/pull/9105#issuecomment-754816274
https://issues.apache.org/jira/browse/ARROW-11009
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org