Skip to content

Commit e40efd8

Browse files
authored
GH-48159 [C++][Gandiva] Projector make is significantly slower after move to OrcJIT (#49063)
### Rationale for this change Reduces LLVM TargetMachine object creation from 3 to 1. This object is expensive to create and the extra copies weren't needed. ### What changes are included in this PR? Refactor the Engine class to only create one target machine and pass that to the necessary functions. Before the change (3 TargetMachines created): First TargetMachine: In Engine::Make(), MakeTargetMachineBuilder() is called, then BuildJIT() is called. Inside LLJITBuilder::create(), when prepareForConstruction() runs, if no DataLayout was set, it calls JTMB->getDefaultDataLayoutForTarget() which creates a temporary TargetMachine just to get the DataLayout. Second TargetMachine: Inside BuildJIT(), when setCompileFunctionCreator is used with the lambda, that lambda calls JTMB.createTargetMachine() to create a TargetMachine for the TMOwningSimpleCompiler. Third TargetMachine: Back in Engine::Make(), after BuildJIT() returns, there's an explicit call to jtmb.createTargetMachine() to create target_machine_ for the Engine. After the change (1 TargetMachine created): The key changes are: Create TargetMachine first: The code now creates the TargetMachine explicitly at the start of the Engine in Engine::Make. That machine is passed to BuildJIT. In BuildJiIT that machine's DataLayout is sent to LLJITBuilder which prevents prepareForConstruction() from calling getDefaultDataLayoutForTarget() (which would create a temporary TargetMachine). Use SimpleCompiler instead of TMOwningSimpleCompiler: SimpleCompiler takes a reference to an existing TargetMachine rather than owning one, so no new TargetMachine is created. A shared_ptr is used to ensure that TargetMachine stays around for the lifetime of the LLJIT instance. ### Are these changes tested? Yes, unit and integration. ### Are there any user-facing changes? No. * GitHub Issue: #48159 Lead-authored-by: logan.riggs@gmail.com <logan.riggs@gmail.com> Co-authored-by: Logan Riggs <logan.riggs@dremio.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
1 parent 8a77885 commit e40efd8

2 files changed

Lines changed: 25 additions & 13 deletions

File tree

cpp/src/gandiva/engine.cc

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -219,28 +219,31 @@ Status UseJITLinkIfEnabled(llvm::orc::LLJITBuilder& jit_builder) {
219219

220220
Result<std::unique_ptr<llvm::orc::LLJIT>> BuildJIT(
221221
llvm::orc::JITTargetMachineBuilder jtmb,
222-
std::optional<std::reference_wrapper<GandivaObjectCache>>& object_cache) {
222+
std::shared_ptr<llvm::TargetMachine> target_machine,
223+
std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache) {
224+
auto data_layout = target_machine->createDataLayout();
225+
223226
llvm::orc::LLJITBuilder jit_builder;
224227

225228
#ifdef JIT_LINK_SUPPORTED
226229
ARROW_RETURN_NOT_OK(UseJITLinkIfEnabled(jit_builder));
227230
#endif
228231

229232
jit_builder.setJITTargetMachineBuilder(std::move(jtmb));
233+
jit_builder.setDataLayout(std::make_optional(data_layout));
234+
230235
if (object_cache.has_value()) {
231236
jit_builder.setCompileFunctionCreator(
232-
[&object_cache](llvm::orc::JITTargetMachineBuilder JTMB)
237+
[tm = std::move(target_machine),
238+
&object_cache](llvm::orc::JITTargetMachineBuilder JTMB)
233239
-> llvm::Expected<std::unique_ptr<llvm::orc::IRCompileLayer::IRCompiler>> {
234-
auto target_machine = JTMB.createTargetMachine();
235-
if (!target_machine) {
236-
return target_machine.takeError();
237-
}
238240
// after compilation, the object code will be stored into the given object
239241
// cache
240-
return std::make_unique<llvm::orc::TMOwningSimpleCompiler>(
241-
std::move(*target_machine), &object_cache.value().get());
242+
return std::make_unique<llvm::orc::SimpleCompiler>(*tm,
243+
&object_cache.value().get());
242244
});
243245
}
246+
244247
auto maybe_jit = jit_builder.create();
245248
ARROW_ASSIGN_OR_RAISE(auto jit,
246249
AsArrowResult(maybe_jit, "Could not create LLJIT instance: "));
@@ -317,7 +320,7 @@ void Engine::InitOnce() {
317320

318321
Engine::Engine(const std::shared_ptr<Configuration>& conf,
319322
std::unique_ptr<llvm::orc::LLJIT> lljit,
320-
std::unique_ptr<llvm::TargetMachine> target_machine, bool cached)
323+
std::shared_ptr<llvm::TargetMachine> target_machine, bool cached)
321324
: context_(std::make_unique<llvm::LLVMContext>()),
322325
lljit_(std::move(lljit)),
323326
ir_builder_(std::make_unique<llvm::IRBuilder<>>(*context_)),
@@ -367,14 +370,21 @@ Result<std::unique_ptr<Engine>> Engine::Make(
367370
std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache) {
368371
std::call_once(llvm_init_once_flag, InitOnce);
369372

373+
// Create the target machine
370374
ARROW_ASSIGN_OR_RAISE(auto jtmb, MakeTargetMachineBuilder(*conf));
371-
ARROW_ASSIGN_OR_RAISE(auto jit, BuildJIT(jtmb, object_cache));
372375
auto maybe_tm = jtmb.createTargetMachine();
373376
ARROW_ASSIGN_OR_RAISE(auto target_machine,
374377
AsArrowResult(maybe_tm, "Could not create target machine: "));
375378

379+
auto shared_target_machine =
380+
std::shared_ptr<llvm::TargetMachine>(std::move(target_machine));
381+
382+
// Build the LLJIT instance
383+
ARROW_ASSIGN_OR_RAISE(auto jit,
384+
BuildJIT(std::move(jtmb), shared_target_machine, object_cache));
385+
376386
std::unique_ptr<Engine> engine{
377-
new Engine(conf, std::move(jit), std::move(target_machine), cached)};
387+
new Engine(conf, std::move(jit), std::move(shared_target_machine), cached)};
378388

379389
ARROW_RETURN_NOT_OK(engine->Init());
380390
return engine;

cpp/src/gandiva/engine.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ class GANDIVA_EXPORT Engine {
9696
private:
9797
Engine(const std::shared_ptr<Configuration>& conf,
9898
std::unique_ptr<llvm::orc::LLJIT> lljit,
99-
std::unique_ptr<llvm::TargetMachine> target_machine, bool cached);
99+
std::shared_ptr<llvm::TargetMachine> target_machine, bool cached);
100100

101101
// Post construction init. This _must_ be called after the constructor.
102102
Status Init();
@@ -130,7 +130,9 @@ class GANDIVA_EXPORT Engine {
130130
bool functions_loaded_ = false;
131131
std::shared_ptr<FunctionRegistry> function_registry_;
132132
std::string module_ir_;
133-
std::unique_ptr<llvm::TargetMachine> target_machine_;
133+
// The lifetime of the TargetMachine is shared with LLJIT. This prevents unnecessary
134+
// duplication of this expensive object.
135+
std::shared_ptr<llvm::TargetMachine> target_machine_;
134136
const std::shared_ptr<Configuration> conf_;
135137
};
136138

0 commit comments

Comments
 (0)