Follow-up from review of #4459 (thread).
ImportedCScalarUdf holds its kernel as Mutex<Box<CometCScalarKernel>>. 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<CometCScalarKernel> would maintain the reference counts and release the instance correctly when the last reference goes. Worth evaluating, along with simply holding Box<CometCScalarKernel> 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).
Follow-up from review of #4459 (thread).
ImportedCScalarUdfholds its kernel asMutex<Box<CometCScalarKernel>>. Everyreturn_typeandinvoke_with_argstakes 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 separateCometCScalarKernelImplbuilt fresh per call.The lock is defensive rather than required: the comment notes DataFusion serializes invocations of a given
ScalarUDFImplanyway, and the FFIDropis what is notSync-safe.@paleolimbot suggested
Arc<CometCScalarKernel>would maintain the reference counts and release the instance correctly when the last reference goes. Worth evaluating, along with simply holdingBox<CometCScalarKernel>directly given the struct is alreadySend + Syncand the kernel is only read after load.Whatever shape this takes, it should preserve the property that the kernel's
releaseruns exactly once, and it should not outlive theLoadedLibrarythat dlopened it (see the field ordering inLoadedLibrary).