apache / apache/datafusion-comet

Remove the per-UDF Mutex serializing Rust UDF batches

Open
#5,252 1 comment 0 reactions 0 assignees View on GitHub
enhancement performance
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.