Conversation
) 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' C++, Gandiva 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. 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.
|
Thanks for opening a pull request! This pull request has been automatically closed because you currently have 4 open pull requests, which is more than the limit of 3. Due to the increase in pull requests opened by AI bots, and in order to keep the review queue manageable, Apache Arrow limits contributors without repository access to at most 3 concurrently open pull requests. This helps make sure each pull request gets the attention it needs and that work in progress does not go stale. Once one of your other open pull requests has been merged or closed, you are welcome to reopen this one. See also: |
|
|
github-actions
Bot
added
awaiting review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
I originally identified this issue in arrow-java 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_ptrllvm::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:
Reviewed before submission by: