This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 47314e8cbf GH-37751: [C++][Gandiva] Avoid registering exported
functions multiple times in gandiva (#37752)
47314e8cbf is described below
commit 47314e8cbf1a2ae8636ac511a368a4e8784397f2
Author: Yue <[email protected]>
AuthorDate: Mon Oct 16 16:54:17 2023 +0800
GH-37751: [C++][Gandiva] Avoid registering exported functions multiple
times in gandiva (#37752)
### Rationale for this change
Try to fix https://github.com/apache/arrow/issues/37751
### What changes are included in this PR?
The exported functions defined in gandiva are registered multiple times
unnecessarily. This PR tries to address this issue.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No
Authored-by: Yue Ni <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
---
cpp/src/gandiva/CMakeLists.txt | 2 ++
cpp/src/gandiva/engine.cc | 3 +++
.../{exported_funcs_registry.cc => exported_funcs.cc} | 18 +++++++++---------
cpp/src/gandiva/exported_funcs.h | 9 ++-------
cpp/src/gandiva/exported_funcs_registry.cc | 11 ++++++++++-
cpp/src/gandiva/exported_funcs_registry.h | 17 +++++++++--------
...ncs_registry.cc => exported_funcs_registry_test.cc} | 12 +++++-------
7 files changed, 40 insertions(+), 32 deletions(-)
diff --git a/cpp/src/gandiva/CMakeLists.txt b/cpp/src/gandiva/CMakeLists.txt
index d2810c39f7..6b6743bc8e 100644
--- a/cpp/src/gandiva/CMakeLists.txt
+++ b/cpp/src/gandiva/CMakeLists.txt
@@ -58,6 +58,7 @@ set(SRC_FILES
expression.cc
expression_registry.cc
exported_funcs_registry.cc
+ exported_funcs.cc
filter.cc
function_ir_builder.cc
function_registry.cc
@@ -237,6 +238,7 @@ add_gandiva_test(internals-test
tree_expr_test.cc
encrypt_utils_test.cc
expr_decomposer_test.cc
+ exported_funcs_registry_test.cc
expression_registry_test.cc
selection_vector_test.cc
lru_cache_test.cc
diff --git a/cpp/src/gandiva/engine.cc b/cpp/src/gandiva/engine.cc
index b6c78da89d..8ebe927437 100644
--- a/cpp/src/gandiva/engine.cc
+++ b/cpp/src/gandiva/engine.cc
@@ -92,6 +92,7 @@
#include "gandiva/configuration.h"
#include "gandiva/decimal_ir.h"
+#include "gandiva/exported_funcs.h"
#include "gandiva/exported_funcs_registry.h"
namespace gandiva {
@@ -103,6 +104,7 @@ std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
static llvm::SmallVector<std::string, 10> cpu_attrs;
+std::once_flag register_exported_funcs_flag;
void Engine::InitOnce() {
DCHECK_EQ(llvm_init, false);
@@ -142,6 +144,7 @@ Engine::Engine(const std::shared_ptr<Configuration>& conf,
cached_(cached) {}
Status Engine::Init() {
+ std::call_once(register_exported_funcs_flag, gandiva::RegisterExportedFuncs);
// Add mappings for global functions that can be accessed from LLVM/IR
module.
AddGlobalMappings();
diff --git a/cpp/src/gandiva/exported_funcs_registry.cc
b/cpp/src/gandiva/exported_funcs.cc
similarity index 66%
copy from cpp/src/gandiva/exported_funcs_registry.cc
copy to cpp/src/gandiva/exported_funcs.cc
index 4c87c4d407..5b04b7c22c 100644
--- a/cpp/src/gandiva/exported_funcs_registry.cc
+++ b/cpp/src/gandiva/exported_funcs.cc
@@ -15,16 +15,16 @@
// specific language governing permissions and limitations
// under the License.
-#include "gandiva/exported_funcs_registry.h"
-
-#include "gandiva/exported_funcs.h"
+#include <gandiva/exported_funcs.h>
+#include <gandiva/exported_funcs_registry.h>
namespace gandiva {
-
-void ExportedFuncsRegistry::AddMappings(Engine* engine) {
- for (auto entry : registered()) {
- entry->AddMappings(engine);
- }
+void RegisterExportedFuncs() {
+ REGISTER_EXPORTED_FUNCS(ExportedStubFunctions);
+ REGISTER_EXPORTED_FUNCS(ExportedContextFunctions);
+ REGISTER_EXPORTED_FUNCS(ExportedTimeFunctions);
+ REGISTER_EXPORTED_FUNCS(ExportedDecimalFunctions);
+ REGISTER_EXPORTED_FUNCS(ExportedStringFunctions);
+ REGISTER_EXPORTED_FUNCS(ExportedHashFunctions);
}
-
} // namespace gandiva
diff --git a/cpp/src/gandiva/exported_funcs.h b/cpp/src/gandiva/exported_funcs.h
index 5a14c52162..82aa020a21 100644
--- a/cpp/src/gandiva/exported_funcs.h
+++ b/cpp/src/gandiva/exported_funcs.h
@@ -17,8 +17,8 @@
#pragma once
-#include <gandiva/exported_funcs_registry.h>
#include <vector>
+#include "gandiva/visibility.h"
namespace gandiva {
@@ -36,36 +36,31 @@ class ExportedFuncsBase {
class ExportedStubFunctions : public ExportedFuncsBase {
void AddMappings(Engine* engine) const override;
};
-REGISTER_EXPORTED_FUNCS(ExportedStubFunctions);
// Class for exporting Context functions
class ExportedContextFunctions : public ExportedFuncsBase {
void AddMappings(Engine* engine) const override;
};
-REGISTER_EXPORTED_FUNCS(ExportedContextFunctions);
// Class for exporting Time functions
class ExportedTimeFunctions : public ExportedFuncsBase {
void AddMappings(Engine* engine) const override;
};
-REGISTER_EXPORTED_FUNCS(ExportedTimeFunctions);
// Class for exporting Decimal functions
class ExportedDecimalFunctions : public ExportedFuncsBase {
void AddMappings(Engine* engine) const override;
};
-REGISTER_EXPORTED_FUNCS(ExportedDecimalFunctions);
// Class for exporting String functions
class ExportedStringFunctions : public ExportedFuncsBase {
void AddMappings(Engine* engine) const override;
};
-REGISTER_EXPORTED_FUNCS(ExportedStringFunctions);
// Class for exporting Hash functions
class ExportedHashFunctions : public ExportedFuncsBase {
void AddMappings(Engine* engine) const override;
};
-REGISTER_EXPORTED_FUNCS(ExportedHashFunctions);
+GANDIVA_EXPORT void RegisterExportedFuncs();
} // namespace gandiva
diff --git a/cpp/src/gandiva/exported_funcs_registry.cc
b/cpp/src/gandiva/exported_funcs_registry.cc
index 4c87c4d407..2c928a7a2a 100644
--- a/cpp/src/gandiva/exported_funcs_registry.cc
+++ b/cpp/src/gandiva/exported_funcs_registry.cc
@@ -22,9 +22,18 @@
namespace gandiva {
void ExportedFuncsRegistry::AddMappings(Engine* engine) {
- for (auto entry : registered()) {
+ for (const auto& entry : *registered()) {
entry->AddMappings(engine);
}
}
+const ExportedFuncsRegistry::list_type& ExportedFuncsRegistry::Registered() {
+ return *registered();
+}
+
+ExportedFuncsRegistry::list_type* ExportedFuncsRegistry::registered() {
+ static list_type registered_list;
+ return ®istered_list;
+}
+
} // namespace gandiva
diff --git a/cpp/src/gandiva/exported_funcs_registry.h
b/cpp/src/gandiva/exported_funcs_registry.h
index 1504f21300..08c45aec6a 100644
--- a/cpp/src/gandiva/exported_funcs_registry.h
+++ b/cpp/src/gandiva/exported_funcs_registry.h
@@ -21,6 +21,7 @@
#include <vector>
#include <gandiva/engine.h>
+#include <gandiva/visibility.h>
namespace gandiva {
@@ -28,7 +29,7 @@ class ExportedFuncsBase;
/// Registry for classes that export functions which can be accessed by
/// LLVM/IR code.
-class ExportedFuncsRegistry {
+class GANDIVA_EXPORT ExportedFuncsRegistry {
public:
using list_type = std::vector<std::shared_ptr<ExportedFuncsBase>>;
@@ -36,19 +37,19 @@ class ExportedFuncsRegistry {
static void AddMappings(Engine* engine);
static bool Register(std::shared_ptr<ExportedFuncsBase> entry) {
- registered().push_back(entry);
+ registered()->emplace_back(std::move(entry));
return true;
}
+ // list all the registered ExportedFuncsBase
+ static const list_type& Registered();
+
private:
- static list_type& registered() {
- static list_type registered_list;
- return registered_list;
- }
+ static list_type* registered();
};
-#define REGISTER_EXPORTED_FUNCS(classname) \
- static bool _registered_##classname = \
+#define REGISTER_EXPORTED_FUNCS(classname) \
+ [[maybe_unused]] static bool _registered_##classname = \
ExportedFuncsRegistry::Register(std::make_shared<classname>())
} // namespace gandiva
diff --git a/cpp/src/gandiva/exported_funcs_registry.cc
b/cpp/src/gandiva/exported_funcs_registry_test.cc
similarity index 80%
copy from cpp/src/gandiva/exported_funcs_registry.cc
copy to cpp/src/gandiva/exported_funcs_registry_test.cc
index 4c87c4d407..6941201e91 100644
--- a/cpp/src/gandiva/exported_funcs_registry.cc
+++ b/cpp/src/gandiva/exported_funcs_registry_test.cc
@@ -16,15 +16,13 @@
// under the License.
#include "gandiva/exported_funcs_registry.h"
-
+#include <gtest/gtest.h>
#include "gandiva/exported_funcs.h"
namespace gandiva {
-
-void ExportedFuncsRegistry::AddMappings(Engine* engine) {
- for (auto entry : registered()) {
- entry->AddMappings(engine);
- }
+TEST(ExportedFuncsRegistry, RegistrationOnlyOnce) {
+ gandiva::RegisterExportedFuncs();
+ auto const& registered_list = ExportedFuncsRegistry::Registered();
+ EXPECT_EQ(registered_list.size(), 6);
}
-
} // namespace gandiva
