apacheGH-37751: [C++][Gandiva] Avoid registering exported functions m…

…ultiple times in gandiva (apache#37752) ### Rationale for this change Try to fix apache#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]>
dgreiss · Feb 17, 2024 · 3a0f6f3 · 3a0f6f3
1 parent b7b98a2
commit 3a0f6f3
Show file tree

Hide file tree

Showing 7 changed files with 84 additions and 16 deletions.
diff --git 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
@@ -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.cc b/cpp/src/gandiva/exported_funcs.cc
@@ -0,0 +1,30 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gandiva/exported_funcs.h>
+#include <gandiva/exported_funcs_registry.h>
+
+namespace gandiva {
+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
@@ -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
@@ -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 &registered_list;
+}
+
 }  // namespace gandiva
diff --git a/cpp/src/gandiva/exported_funcs_registry.h b/cpp/src/gandiva/exported_funcs_registry.h
@@ -21,34 +21,35 @@
 #include <vector>
 
 #include <gandiva/engine.h>
+#include <gandiva/visibility.h>
 
 namespace gandiva {
 
 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>>;
 
   // Add functions from all the registered classes to the engine.
   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_test.cc b/cpp/src/gandiva/exported_funcs_registry_test.cc
@@ -0,0 +1,28 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "gandiva/exported_funcs_registry.h"
+#include <gtest/gtest.h>
+#include "gandiva/exported_funcs.h"
+
+namespace gandiva {
+TEST(ExportedFuncsRegistry, RegistrationOnlyOnce) {
+  gandiva::RegisterExportedFuncs();
+  auto const& registered_list = ExportedFuncsRegistry::Registered();
+  EXPECT_EQ(registered_list.size(), 6);
+}
+}  // namespace gandiva