vvellanki commented on a change in pull request #9833:
URL: https://github.com/apache/arrow/pull/9833#discussion_r608348860
##########
File path:
java/gandiva/src/main/java/org/apache/arrow/gandiva/evaluator/JniLoader.java
##########
@@ -35,7 +37,8 @@
private static volatile JniLoader INSTANCE;
private static volatile long defaultConfiguration = 0L;
- private static volatile long unoptimizedConfiguration = 0L;
+ private static final ConcurrentMap<ConfigurationBuilder.ConfigOptions, Long>
configurationMap
Review comment:
Do we have tests to ensure that this map works as expected?
Using the same Config should return the same long. Release should work as
expected, etc.
##########
File path: cpp/src/gandiva/engine.cc
##########
@@ -133,13 +132,17 @@ Status Engine::Make(const std::shared_ptr<Configuration>&
conf,
// ExecutionEngine but only for the lifetime of the builder. Found by
// inspecting LLVM sources.
std::string builder_error;
- std::unique_ptr<llvm::ExecutionEngine> exec_engine{
- llvm::EngineBuilder(std::move(module))
- .setMCPU(llvm::sys::getHostCPUName())
- .setEngineKind(llvm::EngineKind::JIT)
- .setOptLevel(opt_level)
- .setErrorStr(&builder_error)
- .create()};
+
+ llvm::EngineBuilder engine_builder(std::move(module));
+
+ engine_builder.setEngineKind(llvm::EngineKind::JIT)
+ .setOptLevel(opt_level)
+ .setErrorStr(&builder_error);
+
+ if (conf->target_host_cpu()) {
+ engine_builder.setMCPU(llvm::sys::getHostCPUName());
Review comment:
What is the impact of not setting this? Does LLVM generate sub-optimal
code? or does it internally figure out the CPU and generate appropriate code
for that?
##########
File path:
java/gandiva/src/main/java/org/apache/arrow/gandiva/evaluator/JniLoader.java
##########
@@ -35,7 +37,8 @@
private static volatile JniLoader INSTANCE;
private static volatile long defaultConfiguration = 0L;
- private static volatile long unoptimizedConfiguration = 0L;
+ private static final ConcurrentMap<ConfigurationBuilder.ConfigOptions, Long>
configurationMap
Review comment:
I see a releaseConfig in JNI. Dont we need something similar here to
reduce the size of the Map?
##########
File path: cpp/src/gandiva/engine.cc
##########
@@ -133,13 +132,17 @@ Status Engine::Make(const std::shared_ptr<Configuration>&
conf,
// ExecutionEngine but only for the lifetime of the builder. Found by
// inspecting LLVM sources.
std::string builder_error;
- std::unique_ptr<llvm::ExecutionEngine> exec_engine{
- llvm::EngineBuilder(std::move(module))
- .setMCPU(llvm::sys::getHostCPUName())
- .setEngineKind(llvm::EngineKind::JIT)
- .setOptLevel(opt_level)
- .setErrorStr(&builder_error)
- .create()};
+
+ llvm::EngineBuilder engine_builder(std::move(module));
+
+ engine_builder.setEngineKind(llvm::EngineKind::JIT)
+ .setOptLevel(opt_level)
+ .setErrorStr(&builder_error);
+
+ if (conf->target_host_cpu()) {
+ engine_builder.setMCPU(llvm::sys::getHostCPUName());
Review comment:
This change conflicts with the other change as the call to
getHostCPUName() is done once in the other change
--
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:
[email protected]
