Uh oh!
There was an error while loading. Please reload this page.
GH-40024: [C++][Gandiva] Constructing LLVM module with only necessary functions - #40031
GH-40024: [C++][Gandiva] Constructing LLVM module with only necessary functions#40031niyue wants to merge 9 commits into
Conversation
…ays be loaded, and other part is optional and will be loaded when necessary.
There was a problem hiding this comment.
There are two bitcode files now, the original irhelpers.bc is optionally used, and the mandatory_ir.bc is mandatory and always linked to the LLVM module constructed.
There was a problem hiding this comment.
This is a minor change, after profiling, I found constructing TargetMachine takes some time, and here I tries to re-use the TargetMachine instance created previously.
There was a problem hiding this comment.
So far, all the mandatory IR functions, including "bitMapGetBit"/"bitMapSetBit"/"bitMapValidityGetBit"/"bitMapClearBitIfFalse", are defined in bitmap.cc file
There was a problem hiding this comment.
A new use_cache parameter is added to the benchmark, when use_cache, the expression is the same across iterations (a constant 1 is used), so that code cache will take effect during multiple times of projector construction.
1c61cf0 to
63f5a24CompareThere was a problem hiding this comment.
There are two reasons I make this change for this factory method:
- I realize
GandivaObjectCacheis a higher level concept, to construct it,ExpressionCacheKeyneeds to be used, and the caller needs to be aware of concepts likeSchema/Configuration/Expression, andEngineis lower level and is not aware of these concepts at all. So I switch to use a lower level API (llvm::ObjectCache) instead - in this PR, I tries to avoid constructing too many
TargetMachinefor better performance, aTargetIRAnalysisis passed toEngine's constructor (the Engine only usesTargetIRAnalysisduring optimizing code, but doesn't useTargetMachinedirectly). For some unknown reason (it seems to be a bug of LLVM for me), theTargetIRAnalysisobtained from aTargetMachinerequires theTargetMachineinstance to be alive when running optimization (OptimizeModuleWithNewPassManager), so I change this object_cache to have a default value if nullptr is provided (a cache that does not cache at all), and the only place that constructsEnginewithoutobject_cachewere several unit tests inengine_llvm_test.ccandllvm_generator_test.cc, and the correspondingtarget_machinewill be moved intoTMOwningSimpleCompilerand be alive:
jit_builder.setCompileFunctionCreator(
[&object_cache, &target_machine](llvm::orc::JITTargetMachineBuilder JTMB)
-> llvm::Expected<std::unique_ptr<llvm::orc::IRCompileLayer::IRCompiler>> {
// after compilation, the object code will be stored into the given object
// cache
return std::make_unique<llvm::orc::TMOwningSimpleCompiler>(
std::move(target_machine), object_cache);
});
686eb72 to
6c63093CompareUh oh!
There was an error while loading. Please reload this page.
I added several micro benchmarks to verify the expression compilation performance (previous micro benchmarks primarily focus on execution performance instead of compilation performance). all micro benchmarks
The first 6 benchmarks
The first 6 benchmarks (log scale)
The detailed benchmark statsbefore optimizationafter optimizationConclusion
|
6c63093 to
bfd3f7dComparebfd3f7d to
200d85cCompare…ine can be moved into TMOwningSimplerCompiler, and this avoids the crash during optimization (TargetIRAnalysis for some reason requires its TargetMachine to be alive).
200d85c to
c04f407CompareThank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer. |
dmitry-chirkov-dremio
commented
Dec 17, 2025
@niyue is there still appetite for merging this change? |
niyue
commented
Dec 18, 2025
@dmitry-chirkov-dremio I don’t recall why this PR didn’t move forward at the time, but I can try to pick it up again over the next couple of months and see if I can get it working. |
| llvm::orc::JITTargetMachineBuilder jtmb( | ||
| (llvm::Triple(llvm::sys::getDefaultTargetTriple()))); | ||
| static auto default_target_triple = llvm::sys::getDefaultTargetTriple(); | ||
| llvm::orc::JITTargetMachineBuilder jtmb((llvm::Triple(default_target_triple))); |
There was a problem hiding this comment.
I think this helper whitelist is still incomplete. After this change, a C helper only gets registered if it is either explicitly present in used_functions_ or hardcoded in internal_functions_, but some precompiled IR entry points still call helper stubs transitively.
Two examples I found:
castVARCHAR_decimal128_int64inprecompiled/decimal_wrapper.cccallsgdv_fn_dec_to_stringcastTIMESTAMP_utf8inprecompiled/time.cccallsgdv_fn_time_with_zone
Those helpers are not added to internal_functions_, and they are not top-level expression functions that ExprDecomposer would ever record in used_functions_. That means this optimization can skip registering symbols that the linked IR still needs at runtime.
Can we either keep registering all IR-only helper stubs, or teach this tracking to include transitive C helpers used from precompiled IR?
| arrow::MemoryPool* pool_; | ||
| }; | ||
| TEST_F(TestProjector, TestCastDecimalToString) { |
There was a problem hiding this comment.
This test seems a bit too weak for the regression it is trying to protect. It only checks that Projector::Make(...) succeeds and that the result was not built from cache, but it never evaluates the expression.
Given the risk here is missing helper registration for decimal-to-string code paths, I’d feel better with an actual Evaluate(...) assertion that exercises the generated code. The timezone-aware castTIMESTAMP_utf8 path may also need similar coverage for the same reason.
Rationale for this change
This PR tries to address GH-40024. It keeps track of used functions in Gandiva expressions, and uses that information to avoid defining unused C functions in LLVM module, and avoid loading/linking the LLVM bitcode if no LLVM IR function is used in the expressions. And this helps expression compilation performance.
What changes are included in this PR?
ExprDecomposerhas a new member calledused_functions_to keep track of used functions after visiting the expressions.Engine'sInitprocess is postponed to after all expressions are decomposed, so that all functions used can be obtained before constructing LLVM modules.AddGlobalMappingForFunconly for used functionsirhelpers.bc) into two parts, one for mandatory bitcode, and the other for optional bitcode.Are these changes tested?
Are there any user-facing changes?
No