dmitry-chirkov-dremio commented on code in PR #55282:
URL: https://github.com/apache/arrow/pull/55282#discussion_r4461726673
##########
cpp/src/gandiva/llvm_generator.cc:
##########
@@ -1263,6 +1280,12 @@ LValuePtr
LLVMGenerator::Visitor::BuildFunctionCall(const NativeFunction* func,
auto llvm_return_type = types->IRType(arrow_return_type_id);
DecimalIR decimalIR(generator_->engine_.get());
+ // Only functions that get the execution context can allocate from its arena
(directly
+ // or through a function holder), so those need the arena released after
each row.
+ if (func->NeedsContext()) {
+ has_arena_allocs_ = true;
Review Comment:
Yes, widening this from `kUsesArena` to `kNeedsContext` was intentional.
A context function can use the arena even if it does not return a string.
`levenshtein` returns `int32` but allocates scratch, and external context
functions can do the same. A manual list of arena users can miss a function and
bring the leak back.
I benchmarked few functions:
- `divide` int64 and int32
- `add` int64 as a no-context control
- `castVARCHAR(s, 8)`
- longer expressions with 10–100 functions
- `ltrim`, `left`, `substr`, `upper`, `concat`
- `length(upper(s))`
- `levenshtein`
`divide` was chosen because it is a very cheap context function. That makes
any added per-row overhead easiest to see.
With the inline arena-used check:
- `divide` int64: `0.53` → `0.54 ns/row`
- `divide` int32: `0.54` → `0.54 ns/row`
- `castVARCHAR(s, 8)`: `6.82` → `6.54 ns/row`
The longer expressions stayed within about `±0.3 ns/row`. I did not run a
standalone benchmark for every function, and `locate` specifically was not
measured.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
