This is an automated email from the ASF dual-hosted git repository.
kou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 2db03408aae GH-51663: [C++][Gandiva] Race condition with Gandiva
expressions (#51665)
2db03408aae is described below
commit 2db03408aaefad7edaf48831b37ed6047a80bce4
Author: Logan Riggs <[email protected]>
AuthorDate: Fri Oct 23 06:53:02 2026 -0700
GH-51663: [C++][Gandiva] Race condition with Gandiva expressions (#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:
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:
```text
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 `std::shared_ptr<llvm::MemoryBuffer>` rather than a
`GandivaObjectCache&`. `Engine::SetLLVMObjectCache()` passes a copy of the
buffer to `addObjectFile()`, as the old `GandivaObjectCache::getObject()` did,
because the shared cache still owns the original buffer.
No new locking — this removes the window rather than serializing around it.
### Are these changes tested?
This adds a new deterministic regression test,
`cpp/src/gandiva/tests/cache_race_test.cc`, which is part of
`gandiva-projector-test`.
The test doesn't depend on thread timing. It registers an
`ExportedFuncsBase` hook that inserts an entry for the key being built into the
shared cache during `Engine::Init()`. That is after `Make()`'s first cache
lookup and before the (pre-fix) second lookup in `SetLLVMObjectCache()`, so the
race window is reproduced on every run. It covers both `Projector::Make()` and
`Filter::Make()`, and checks that the built projector/filter evaluates
correctly.
Verified the 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
Authored-by: [email protected] <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
---
cpp/src/gandiva/engine.cc | 11 +-
cpp/src/gandiva/engine.h | 8 +-
cpp/src/gandiva/filter.cc | 7 +-
cpp/src/gandiva/llvm_generator.cc | 5 +-
cpp/src/gandiva/llvm_generator.h | 7 +-
cpp/src/gandiva/projector.cc | 7 +-
cpp/src/gandiva/tests/CMakeLists.txt | 1 +
cpp/src/gandiva/tests/cache_race_test.cc | 212 +++++++++++++++++++++++++++++++
8 files changed, 245 insertions(+), 13 deletions(-)
diff --git a/cpp/src/gandiva/engine.cc b/cpp/src/gandiva/engine.cc
index 205d2f866b2..e208bb3dc0e 100644
--- a/cpp/src/gandiva/engine.cc
+++ b/cpp/src/gandiva/engine.cc
@@ -304,9 +304,14 @@ void RemoveBuildTargetAttributes(llvm::Module& module) {
} // namespace
-Status Engine::SetLLVMObjectCache(GandivaObjectCache& object_cache) {
- auto cached_buffer = object_cache.getObject(nullptr);
- if (cached_buffer) {
+Status Engine::SetLLVMObjectCache(
+ const std::shared_ptr<llvm::MemoryBuffer>& prev_cached_obj) {
+ if (prev_cached_obj) {
+ // Copy rather than hand over the cache's own buffer -- addObjectFile takes
+ // ownership and may free it, but the shared cache map holds a reference
to the
+ // same object too.
+ auto cached_buffer = llvm::MemoryBuffer::getMemBufferCopy(
+ prev_cached_obj->getBuffer(), prev_cached_obj->getBufferIdentifier());
auto error = lljit_->addObjectFile(std::move(cached_buffer));
if (error) {
return Status::CodeGenError("Failed to add cached object file to LLJIT:
",
diff --git a/cpp/src/gandiva/engine.h b/cpp/src/gandiva/engine.h
index 20165787cb6..23fc366fe15 100644
--- a/cpp/src/gandiva/engine.h
+++ b/cpp/src/gandiva/engine.h
@@ -74,8 +74,12 @@ class GANDIVA_EXPORT Engine {
/// Optimise and compile the module.
Status FinalizeModule();
- /// Set LLVM ObjectCache.
- Status SetLLVMObjectCache(GandivaObjectCache& object_cache);
+ /// Pre-load the LLJIT with an already-compiled object for the current
module, if
+ /// one was found by the caller's own cache lookup. Takes the lookup result
directly
+ /// rather than re-querying the cache, so this can never disagree with the
\p cached
+ /// flag passed to \p Make -- avoiding the load being based on a newer cache
state
+ /// than the one that decided whether Make() would still build (and load)
fresh IR.
+ Status SetLLVMObjectCache(const std::shared_ptr<llvm::MemoryBuffer>&
prev_cached_obj);
/// Get the compiled function corresponding to the irfunction.
Result<void*> CompiledFunction(const std::string& function);
diff --git a/cpp/src/gandiva/filter.cc b/cpp/src/gandiva/filter.cc
index 8a270cfdc06..815516b6969 100644
--- a/cpp/src/gandiva/filter.cc
+++ b/cpp/src/gandiva/filter.cc
@@ -76,8 +76,11 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
ARROW_RETURN_NOT_OK(expr_validator.Validate(condition));
}
- // Set the object cache for LLVM
- ARROW_RETURN_NOT_OK(llvm_gen->SetLLVMObjectCache(obj_cache));
+ // Pre-load the already-compiled object from the cache lookup above, if
there was
+ // one. Reuses prev_cached_obj instead of letting SetLLVMObjectCache
re-query the
+ // shared cache, so this can never disagree with the is_cached snapshot that
+ // decided whether Build() below still compiles.
+ ARROW_RETURN_NOT_OK(llvm_gen->SetLLVMObjectCache(prev_cached_obj));
ARROW_RETURN_NOT_OK(llvm_gen->Build({condition},
SelectionVector::Mode::MODE_NONE));
diff --git a/cpp/src/gandiva/llvm_generator.cc
b/cpp/src/gandiva/llvm_generator.cc
index 7caafa58369..7aa91c4dd6b 100644
--- a/cpp/src/gandiva/llvm_generator.cc
+++ b/cpp/src/gandiva/llvm_generator.cc
@@ -64,8 +64,9 @@ LLVMGenerator::GetCache() {
return shared_cache;
}
-Status LLVMGenerator::SetLLVMObjectCache(GandivaObjectCache& object_cache) {
- return engine_->SetLLVMObjectCache(object_cache);
+Status LLVMGenerator::SetLLVMObjectCache(
+ const std::shared_ptr<llvm::MemoryBuffer>& prev_cached_obj) {
+ return engine_->SetLLVMObjectCache(prev_cached_obj);
}
Status LLVMGenerator::Add(const ExpressionPtr expr, const FieldDescriptorPtr
output) {
diff --git a/cpp/src/gandiva/llvm_generator.h b/cpp/src/gandiva/llvm_generator.h
index c6e4e821dfe..0c796b4b2a2 100644
--- a/cpp/src/gandiva/llvm_generator.h
+++ b/cpp/src/gandiva/llvm_generator.h
@@ -58,8 +58,11 @@ class GANDIVA_EXPORT LLVMGenerator {
static std::shared_ptr<Cache<ExpressionCacheKey,
std::shared_ptr<llvm::MemoryBuffer>>>
GetCache();
- /// \brief Set LLVM ObjectCache.
- Status SetLLVMObjectCache(GandivaObjectCache& object_cache);
+ /// \brief Pre-load the engine with an already-compiled object for the
current
+ /// module, if the caller's own cache lookup found one. See the equivalent
Engine
+ /// method for why this takes the lookup result directly instead of
re-querying
+ /// the cache.
+ Status SetLLVMObjectCache(const std::shared_ptr<llvm::MemoryBuffer>&
prev_cached_obj);
/// \brief Build the code for the expression trees for default mode with a
LLVM
/// ObjectCache. Each element in the vector represents an expression tree
diff --git a/cpp/src/gandiva/projector.cc b/cpp/src/gandiva/projector.cc
index ec0302146ff..b6c11ce0477 100644
--- a/cpp/src/gandiva/projector.cc
+++ b/cpp/src/gandiva/projector.cc
@@ -94,8 +94,11 @@ Status Projector::Make(SchemaPtr schema, const
ExpressionVector& exprs,
}
}
- // Set the object cache for LLVM
- ARROW_RETURN_NOT_OK(llvm_gen->SetLLVMObjectCache(obj_cache));
+ // Pre-load the already-compiled object from the cache lookup above, if
there was
+ // one. Reuses prev_cached_obj instead of letting SetLLVMObjectCache
re-query the
+ // shared cache, so this can never disagree with the is_cached snapshot that
+ // decided whether Build() below still compiles.
+ ARROW_RETURN_NOT_OK(llvm_gen->SetLLVMObjectCache(prev_cached_obj));
ARROW_RETURN_NOT_OK(llvm_gen->Build(exprs, selection_vector_mode));
diff --git a/cpp/src/gandiva/tests/CMakeLists.txt
b/cpp/src/gandiva/tests/CMakeLists.txt
index be635e16d3a..7746645ad9a 100644
--- a/cpp/src/gandiva/tests/CMakeLists.txt
+++ b/cpp/src/gandiva/tests/CMakeLists.txt
@@ -19,6 +19,7 @@ add_gandiva_test(projector-test
SOURCES
binary_test.cc
boolean_expr_test.cc
+ cache_race_test.cc
date_time_test.cc
decimal_alignment_test.cc
decimal_single_test.cc
diff --git a/cpp/src/gandiva/tests/cache_race_test.cc
b/cpp/src/gandiva/tests/cache_race_test.cc
new file mode 100644
index 00000000000..ae7677105ec
--- /dev/null
+++ b/cpp/src/gandiva/tests/cache_race_test.cc
@@ -0,0 +1,212 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Deterministic regression test for GH-51663.
+//
+// Projector::Make() and Filter::Make() read the shared expression cache once
to decide
+// `is_cached`. Before the fix, SetLLVMObjectCache() performed a *second*,
independent
+// read of the same key. A thread that saw a miss on the first read and a hit
on the
+// second one would both pre-load the cached object (defining e.g. "expr_0_0"
in its
+// JITDylib) and add its own compiled IR module defining the same symbol, so
LLVM ORC's
+// duplicate-symbol detection fired. (Originally reported against the Java
bindings as
+// apache/arrow-java#601.)
+
+#include <gtest/gtest.h>
+
+#include <atomic>
+#include <memory>
+#include <utility>
+#include <vector>
+
+#include "arrow/memory_pool.h"
+#include "gandiva/cache.h"
+#include "gandiva/configuration.h"
+#include "gandiva/exported_funcs.h"
+#include "gandiva/exported_funcs_registry.h"
+#include "gandiva/expression_cache_key.h"
+#include "gandiva/filter.h"
+#include "gandiva/llvm_generator.h"
+#include "gandiva/projector.h"
+#include "gandiva/tests/test_util.h"
+#include "gandiva/tree_expr_builder.h"
+
+namespace gandiva {
+
+using arrow::boolean;
+using arrow::int32;
+
+namespace {
+
+using ObjectCacheType = Cache<ExpressionCacheKey,
std::shared_ptr<llvm::MemoryBuffer>>;
+
+struct PendingInjection {
+ std::shared_ptr<ObjectCacheType> cache;
+ std::shared_ptr<ExpressionCacheKey> key;
+ std::shared_ptr<llvm::MemoryBuffer> object;
+ std::atomic<bool> armed{false};
+};
+
+PendingInjection* GetPendingInjection() {
+ static PendingInjection instance;
+ return &instance;
+}
+
+// Registered once; fires only while armed.
+class CacheRaceInjector : public ExportedFuncsBase {
+ public:
+ arrow::Status AddMappings(Engine* engine) const override {
+ auto* pending = GetPendingInjection();
+ // exchange() disarms, so this injects exactly once per arming even though
the
+ // registry keeps calling us on every subsequent Engine::Init().
+ if (pending->armed.exchange(false)) {
+ pending->cache->PutObjectCode(*pending->key, pending->object);
+ }
+ return arrow::Status::OK();
+ }
+};
+
+REGISTER_EXPORTED_FUNCS(CacheRaceInjector);
+
+// Arms the injection that will run inside the next Make()'s engine
construction.
+void ArmInjection(std::shared_ptr<ObjectCacheType> cache,
+ std::shared_ptr<ExpressionCacheKey> key,
+ std::shared_ptr<llvm::MemoryBuffer> object) {
+ auto* pending = GetPendingInjection();
+ pending->cache = std::move(cache);
+ pending->key = std::move(key);
+ pending->object = std::move(object);
+ pending->armed.store(true);
+}
+
+} // namespace
+
+class TestCacheRace : public ::testing::Test {
+ public:
+ void SetUp() { pool_ = arrow::default_memory_pool(); }
+
+ void TearDown() { GetPendingInjection()->armed.store(false); }
+
+ protected:
+ arrow::MemoryPool* pool_;
+};
+
+TEST_F(TestCacheRace, TestProjectorCacheInsertedDuringMake) {
+ auto field0 = field("f0", int32());
+ auto field1 = field("f1", int32());
+ auto schema = arrow::schema({field0, field1});
+ auto field_sum = field("add", int32());
+ auto sum_expr = TreeExprBuilder::MakeExpression("add", {field0, field1},
field_sum);
+ ExpressionVector exprs = {sum_expr};
+
+ // Two separately built configurations with identical content: same hash,
different
+ // keys.
+ auto config_a = ConfigurationBuilder().build();
+ auto config_b = ConfigurationBuilder().build();
+
+ // Build once under config_a so the expression is cached.
+ std::shared_ptr<Projector> warm_projector;
+ ASSERT_OK(Projector::Make(schema, exprs, config_a, &warm_projector));
+
+ auto cache = LLVMGenerator::GetCache();
+ ExpressionCacheKey key_a(schema, config_a, exprs,
SelectionVector::Mode::MODE_NONE);
+ auto cached_object = cache->GetObjectCode(key_a);
+ ASSERT_NE(cached_object, nullptr) << "expected the first Make() to populate
the cache";
+
+ // config_b has never been built, so Make()'s first read is a guaranteed
miss.
+ auto key_b = std::make_shared<ExpressionCacheKey>(schema, config_b, exprs,
+
SelectionVector::Mode::MODE_NONE);
+ ASSERT_EQ(cache->GetObjectCode(*key_b), nullptr) << "key_b should start
uncached";
+
+ // Fires inside Engine::Init(), i.e. after the first read and before the
(pre-fix)
+ // second.
+ ArmInjection(cache, key_b, cached_object);
+
+ std::shared_ptr<Projector> projector;
+ auto status = Projector::Make(schema, exprs, config_b, &projector);
+
+ ASSERT_OK(status) << "a cache entry appearing mid-Make() must not produce a
duplicate "
+ "symbol -- see GH-51663";
+ ASSERT_NE(projector, nullptr);
+ EXPECT_FALSE(GetPendingInjection()->armed.load())
+ << "injection never fired; the window this test targets was not reached";
+
+ // The projector must also be usable: a silently wrong module is as bad as
an error.
+ int num_records = 4;
+ auto array0 = MakeArrowArrayInt32({1, 2, 3, 4}, {true, true, true, true});
+ auto array1 = MakeArrowArrayInt32({11, 13, 15, 17}, {true, true, true,
true});
+ auto exp_sum = MakeArrowArrayInt32({12, 15, 18, 21}, {true, true, true,
true});
+ auto in_batch = arrow::RecordBatch::Make(schema, num_records, {array0,
array1});
+
+ arrow::ArrayVector outputs;
+ ASSERT_OK(projector->Evaluate(*in_batch, pool_, &outputs));
+ EXPECT_ARROW_ARRAY_EQUALS(exp_sum, outputs.at(0));
+}
+
+TEST_F(TestCacheRace, TestFilterCacheInsertedDuringMake) {
+ auto field0 = field("g0", int32());
+ auto field1 = field("g1", int32());
+ auto schema = arrow::schema({field0, field1});
+
+ // g0 + g1 < 10
+ auto node_f0 = TreeExprBuilder::MakeField(field0);
+ auto node_f1 = TreeExprBuilder::MakeField(field1);
+ auto sum_func = TreeExprBuilder::MakeFunction("add", {node_f0, node_f1},
int32());
+ auto literal_10 = TreeExprBuilder::MakeLiteral(static_cast<int32_t>(10));
+ auto less_than_10 =
+ TreeExprBuilder::MakeFunction("less_than", {sum_func, literal_10},
boolean());
+ auto condition = TreeExprBuilder::MakeCondition(less_than_10);
+
+ auto config_a = ConfigurationBuilder().build();
+ auto config_b = ConfigurationBuilder().build();
+
+ std::shared_ptr<Filter> warm_filter;
+ ASSERT_OK(Filter::Make(schema, condition, config_a, &warm_filter));
+
+ auto cache = LLVMGenerator::GetCache();
+ // Filter::Make uses the Condition overload of the key.
+ Condition condition_for_key = *(condition.get());
+ ExpressionCacheKey key_a(schema, config_a, condition_for_key);
+ auto cached_object = cache->GetObjectCode(key_a);
+ ASSERT_NE(cached_object, nullptr) << "expected the first Make() to populate
the cache";
+
+ auto key_b = std::make_shared<ExpressionCacheKey>(schema, config_b,
condition_for_key);
+ ASSERT_EQ(cache->GetObjectCode(*key_b), nullptr) << "key_b should start
uncached";
+
+ ArmInjection(cache, key_b, cached_object);
+
+ std::shared_ptr<Filter> filter;
+ auto status = Filter::Make(schema, condition, config_b, &filter);
+
+ ASSERT_OK(status) << "a cache entry appearing mid-Make() must not produce a
duplicate "
+ "symbol -- see GH-51663";
+ ASSERT_NE(filter, nullptr);
+ EXPECT_FALSE(GetPendingInjection()->armed.load())
+ << "injection never fired; the window this test targets was not reached";
+
+ int num_records = 5;
+ auto array0 = MakeArrowArrayInt32({1, 2, 3, 4, 6}, {true, true, true, false,
true});
+ auto array1 = MakeArrowArrayInt32({5, 9, 6, 17, 3}, {true, true, false,
true, true});
+ auto exp = MakeArrowArrayUint16({0, 4});
+ auto in_batch = arrow::RecordBatch::Make(schema, num_records, {array0,
array1});
+
+ std::shared_ptr<SelectionVector> selection_vector;
+ ASSERT_OK(SelectionVector::MakeInt16(num_records, pool_, &selection_vector));
+ ASSERT_OK(filter->Evaluate(*in_batch, selection_vector));
+ EXPECT_ARROW_ARRAY_EQUALS(exp, selection_vector->ToArray());
+}
+
+} // namespace gandiva
