apache / apache/datafusion-comet
Remove the per-UDF Mutex serializing Rust UDF batches
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
Follow-up from review of #4459 ([thread](https://github.com/apache/datafusion-comet/pull/4459#discussion_r3712474949)).
`ImportedCScalarUdf` holds its kernel as `Mutex>`. Every `return_type` and `invoke_with_args` takes that lock, so all concurrent batches for a given UDF in a process serialize on one mutex, even though the kernel is logically immutable after load and the per-execution state already lives in a separate `CometCScalarKernelImpl` built fresh per call.
The lock is defensive rather than required: the comment notes DataFusion serializes invocations of a given `ScalarUDFImpl` anyway, and the FFI `Drop` is what is not `Sync`-safe.
@paleolimbot suggested `Arc` would maintain the reference counts and release the instance correctly when the last reference goes. Worth evaluating, along with simply holding `Box` directly given the struct is already `Send + Sync` and the kernel is only read after load.
Whatever shape this takes, it should preserve the property that the kernel's `release` runs exactly once, and it should not outlive the `LoadedLibrary` that dlopened it (see the field ordering in `LoadedLibrary`).
Contributor guide
Research direction
Read the ImportedCScalarUdf and CometCScalarKernel definitions, then inspect LoadedLibrary's field ordering and the existing review thread from #4459. Compare the Box and Arc ownership options, checking Send/Sync, FFI Drop behavior, and library lifetime. Done means concurrent batches no longer serialize on the per-UDF mutex, release runs exactly once, and the kernel cannot outlive its LoadedLibrary.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend, performance
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100