DX-125697: Gandiva expression-compilation lock contention limits single-large-node parallelism

[lriggs]

Problem

Projector::Make() and Filter::Make() read the shared expression cache once to decide
is_cached, then call SetLLVMObjectCache(), which performed 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:

1. take the is_cached == false path, generating expr_0_0 into its IR module, and
2. 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'

This is the root cause behind apache/arrow-java's coarser workaround of making
Projector.make()/Filter.make() static synchronized, which serialized all LLVM compilation
in a JVM to close a race that only ever involved a single cache key.

Fix

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. Cache hits are not
lost: the GandivaObjectCache is still installed on the compile layer, so
SimpleCompiler::tryToLoadFromObjectCache still picks up a late-arriving entry, via the one path
that cannot double-define.

Testing

New cpp/src/gandiva/tests/concurrent_make_test.cc (in the projector-test target): N threads
(≥8, ≥2× hardware concurrency) released from a shared gate all call Projector::Make /
Filter::Make on an identical cache key, over 25 iterations, each with a fresh schema so every
round starts from a genuine miss. Every Make is ASSERT_OK'd and every resulting
projector/filter is evaluated and its output checked — no existing test evaluated a projector
built from the cache-hit path.

• Reverted onto the parent commit, both tests fail with the duplicate-symbol error above
  (observed at iterations 21 and 4).
• With the fix, both pass and ctest -L gandiva-tests is green (4/4 targets).

Note for reviewers

Engine::SetLLVMObjectCache and LLVMGenerator::SetLLVMObjectCache are GANDIVA_EXPORT and
their headers are installed, so this changes an exported signature. In-tree there are no other
callers, and the Gandiva JNI only uses Projector::Make/Filter::Make, which are unchanged.

[xxlaykxx]

🟡 Verdict: Minor issues found — 0 must fix, 2 should fix

The fix itself looks correct: on the hit path FinalizeModule() skips addIRModule, so the only expr_* definition is the pre-loaded object; on the miss path a late-arriving cache entry is picked up by SimpleCompiler::tryToLoadFromObjectCache instead of compiling, so it can't double-define. It also fixes the reverse race (entry evicted between the two reads → is_cached == true but nothing loaded → "Failed to look up function"), since prev_cached_obj pins the buffer — might be worth mentioning in the description.

Inspect first:

• cpp/src/gandiva/engine.cc — core change to how the cached object is pre-loaded
• cpp/src/gandiva/tests/concurrent_make_test.cc — heavy, unbounded concurrency in CI

Change walkthrough (8 files)

File	Change
cpp/src/gandiva/engine.cc	SetLLVMObjectCache takes the caller's lookup result and copies it into the LLJIT instead of re-querying the cache
cpp/src/gandiva/engine.h	Signature + doc update
cpp/src/gandiva/llvm_generator.cc	Pass-through signature change
cpp/src/gandiva/llvm_generator.h	Signature + doc update
cpp/src/gandiva/projector.cc	Pass prev_cached_obj instead of obj_cache
cpp/src/gandiva/filter.cc	Pass prev_cached_obj instead of obj_cache
cpp/src/gandiva/tests/CMakeLists.txt	Add new test source to projector-test
cpp/src/gandiva/tests/concurrent_make_test.cc	New concurrent same-cache-key Projector/Filter Make + evaluate tests

🟡 Should fix:

• [Tests] NumThreads() is unbounded — 2 × hardware_concurrency means ~128 concurrent LLJIT builds on a 64-core runner, × 25 iterations × 2 tests; risk of CI timeout/OOM. Cap it (e.g. 32). (cpp/src/gandiva/tests/concurrent_make_test.cc:79)
• [Scope] The title promises relief from lock contention, but this PR only removes the C++ race; parallelism won't improve until the Java static synchronized on Projector.make()/Filter.make() (apache/arrow-java#601) is removed. Please retitle, or link the arrow-java follow-up so DX-125697 isn't closed prematurely.

Minor: code comments reference "apacheGH-601", which in an apache/arrow-derived repo reads as apache#601 — consider apache/arrow-java#601 or DX-125697.

---

AI-assisted analysis

[dmitry-chirkov-dremio]

I'm approving code changes to unblock the merge.
I still have feedback for tests captured in apache#51665 so please follow up there.

[akravchukdremio]

Looks good