lriggs opened a new pull request, #51665: URL: https://github.com/apache/arrow/pull/51665
### Rationale for this change I originally identified [this issue in arrow-java](https://github.com/apache/arrow-java/issues/601) and fixed it on the arrow-java side with a coarse grained lock on the Projector and Filter Make methods. That fixed the issue but adds some undesirable overhead in certain multi threaded workflows. This issue captures the deeper problem that is occurring in the C++ side of Gandiva and was not touched by my previous fix though it is prevented from happening. I have a proposed solution for this and believe it is a better overall solution. Projector::Make() and Filter::Make() read the shared expression cache once to decide the is_cached status and then call SetLLVMObjectCache(). That performs its own second, unsynchronized read of the same key before pre-loading a cached object into the LLJIT. Those two reads could disagree. If another thread compiling the identical (schema, expressions, selection vector mode, configuration) tuple inserted between them, the first thread would: take the is_cached == false path, generating expr_0_0 into its IR module, and also see a hit on the second read and addObjectFile() a cached object that defines expr_0_0 as well. Both then call JITDylib::define for the same symbol in the same JITDylib, and ORC's duplicate-symbol detection fires: CodeGenError in Gandiva: Failed to add IR module to LLJIT: In gdv_module_..., duplicate definition of symbol 'expr_0_0' ### What changes are included in this PR? Thread the single cache-lookup result (prev_cached_obj) that already determined is_cached directly into SetLLVMObjectCache(), instead of letting it perform an independent second lookup. Engine::SetLLVMObjectCache and LLVMGenerator::SetLLVMObjectCache now take the resolved shared_ptr<llvm::MemoryBuffer> rather than a GandivaObjectCache&. No new locking — this removes the window rather than serializing around it. ### Are these changes tested? New cpp/src/gandiva/tests/concurrent_make_test.cc. Verified tests fail with the duplicate-symbol error without this fix. Verified internally with our product stress tests after removing the arrow-java side of the fix. Once this change is in C++ I can make the java change to remove the synchronization. ### Are there any user-facing changes? **This PR includes breaking changes to public APIs.** Engine::SetLLVMObjectCache and LLVMGenerator::SetLLVMObjectCache are GANDIVA_EXPORT so this changes an exported signature. In-tree there are no other callers, and the Gandiva Java side JNI only uses Projector::Make/Filter::Make, which are unchanged. ### Was AI used for this PR? AI was used to diagnose the problem, write code and tests over several iterations before being reviewed by two human developers. A human ran several manual and regression tests against the change. One human reviewed and heavily edited the PR description and also wrote some himself. **PR code and description written by:** - [X] Human - [X] AI **Reviewed before submission by:** - [X] Human - [X] AI - [ ] Not reviewed * GitHub Issue: #51663 -- 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]
