Skip to content

Commit 12e79a2

Browse files
jasonmfehrImpala Public Jenkins
authored andcommitted
IMPALA-14863: Fixes Potential OOM During Row Batch Processing
Impala checks at the end of every row batch to determine if the query or process memory limit has been exceeded. If the batch processing requires evaluating expressions that allocate memory for their results (such as UPPER() or LOWER()), that memory is allocated without checking any memory limits. If a large number of these expression evaluations are performed during a single row batch, memory will be consumed until the process memory limit is reached. At that point, the executor begins failing other queries with cannot allocate memory errors as they request memory. However, the row batch processing continues to run since it does no memory limit checking during batch processing. This patch determines a limit for the amount of memory that conjunct expression evaluation results can consume as a percentage of the lowest soft memory limit for the query execution. Before evaluating conjuncts for each join probe row, the expression results memory pool size is cleared if its allocated memory has exceeded this threshold. Clearing the memory pool does not result in memory being freed but instead marks all memory in the pool as available for re-use. Testing accomplished by a new custom cluster test that replicates the query that revealed this issue. Regression functionality testing performed by running existing test suite. Regression performance testing performed by running AB test job. Generated-by: Github Copilot (GPT-5.3-Codex) Change-Id: Ic54b5c39e1388681275681f22e61b27728dba5af Reviewed-on: http://gerrit.cloudera.org:8080/24159 Reviewed-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com> Tested-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
1 parent b8be513 commit 12e79a2

8 files changed

Lines changed: 251 additions & 3 deletions

be/src/codegen/llvm-codegen-test.cc

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ using std::unique_ptr;
4141

4242
namespace impala {
4343

44+
typedef void (*TestLoopFn)(int, int64_t*);
45+
4446
class CodegenFnPtrTest : public testing:: Test {
4547
protected:
4648
typedef int (*FnPtr)(int);
@@ -234,7 +236,6 @@ llvm::Function* CodegenInnerLoop(
234236
TEST_F(LlvmCodeGenTest, ReplaceFnCall) {
235237
const string loop_call_name("_Z21DefaultImplementationPl");
236238
const string loop_name("_Z8TestLoopiPl");
237-
typedef void (*TestLoopFn)(int, int64_t*);
238239

239240
string module_file;
240241
PathBuilder::GetFullPath("llvm-ir/test-loop.bc", &module_file);
@@ -309,6 +310,57 @@ TEST_F(LlvmCodeGenTest, ReplaceFnCall) {
309310
codegen->Close();
310311
}
311312

313+
// This test loads a precompiled IR file (compiled from testdata/llvm/test-loop.cc).
314+
// The test contains two functions, an outer loop function and an inner loop function.
315+
// The outer loop calls the inner loop function.
316+
// The test will
317+
// 1. Create a LlvmCodegen object from the precompiled file.
318+
// 2. Replaces the inner loop with with an implementation that increments the counter
319+
// each time it is called.
320+
// 3. Remove the inner loop call from the outer loop.
321+
// 4. Compile and run all loops and make sure the inner loop is not called.
322+
TEST_F(LlvmCodeGenTest, RemoveFnCall) {
323+
const string loop_call_name("_Z21DefaultImplementationPl");
324+
const string loop_name("_Z8TestLoopiPl");
325+
326+
string module_file;
327+
PathBuilder::GetFullPath("llvm-ir/test-loop.bc", &module_file);
328+
329+
// Part 1: Load the module and make sure everything is loaded correctly.
330+
scoped_ptr<LlvmCodeGen> codegen;
331+
ASSERT_OK(CreateFromFile(module_file.c_str(), &codegen));
332+
EXPECT_TRUE(codegen.get() != nullptr);
333+
334+
llvm::Function* loop = codegen->GetFunction(loop_name, false);
335+
EXPECT_TRUE(loop != nullptr);
336+
EXPECT_EQ(loop->arg_size(), 2);
337+
338+
// Part 2: Replace the default inner loop implementation that does not modify counter
339+
// with a function that increments the counter each time it is called.
340+
llvm::Function* jitted_loop_call = CodegenInnerLoop(codegen.get(), 1);
341+
llvm::Function* jitted_loop = codegen->CloneFunction(loop);
342+
int num_replaced =
343+
codegen->ReplaceCallSites(jitted_loop, jitted_loop_call, loop_call_name);
344+
EXPECT_EQ(1, num_replaced);
345+
EXPECT_TRUE(VerifyFunction(codegen.get(), jitted_loop));
346+
347+
// Part 3: Remove the inner loop call from the outer loop.
348+
EXPECT_EQ(1, codegen->RemoveCallSites(jitted_loop, "JittedInnerLoop"));
349+
350+
// Part 4: compile all the function and run it.
351+
CodegenFnPtr<TestLoopFn> new_loop;
352+
AddFunctionToJit(codegen.get(), jitted_loop, &new_loop);
353+
ASSERT_OK(LlvmCodeGenTest::FinalizeModule(codegen.get()));
354+
ASSERT_TRUE(new_loop.load() != nullptr);
355+
356+
int64_t counter = 0;
357+
TestLoopFn new_loop_fn = new_loop.load();
358+
new_loop_fn(5, &counter);
359+
EXPECT_EQ(0, counter);
360+
361+
codegen->Close();
362+
}
363+
312364
// Test function for c++/ir interop for strings. Function will do:
313365
// int StringTest(StringValue* strval) {
314366
// strval->ptr[0] = 'A';

be/src/codegen/llvm-codegen.cc

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -991,8 +991,8 @@ Status LlvmCodeGen::LoadFunction(const TFunction& fn, const string& symbol,
991991
int LlvmCodeGen::ReplaceCallSites(
992992
llvm::Function* caller, llvm::Function* new_fn, const string& target_name) {
993993
DCHECK(!is_compiled_);
994-
DCHECK(caller->getParent() == module_);
995994
DCHECK(caller != NULL);
995+
DCHECK(caller->getParent() == module_);
996996
DCHECK(new_fn != NULL);
997997
DCHECK(find(handcrafted_functions_.begin(), handcrafted_functions_.end(), new_fn)
998998
== handcrafted_functions_.end()
@@ -1012,8 +1012,8 @@ int LlvmCodeGen::ReplaceCallSites(
10121012
int LlvmCodeGen::ReplaceCallSitesWithValue(
10131013
llvm::Function* caller, llvm::Value* replacement, const string& target_name) {
10141014
DCHECK(!is_compiled_);
1015-
DCHECK(caller->getParent() == module_);
10161015
DCHECK(caller != NULL);
1016+
DCHECK(caller->getParent() == module_);
10171017
DCHECK(replacement != NULL);
10181018

10191019
vector<llvm::CallInst*> call_sites;
@@ -1032,6 +1032,21 @@ int LlvmCodeGen::ReplaceCallSitesWithBoolConst(llvm::Function* caller, bool cons
10321032
return ReplaceCallSitesWithValue(caller, replacement, target_name);
10331033
}
10341034

1035+
int LlvmCodeGen::RemoveCallSites(llvm::Function* caller, const std::string& target_name) {
1036+
DCHECK(!is_compiled_);
1037+
DCHECK(caller != NULL);
1038+
DCHECK(caller->getParent() == module_);
1039+
1040+
vector<llvm::CallInst*> call_sites;
1041+
FindCallSites(caller, target_name, &call_sites);
1042+
1043+
for (llvm::CallInst* call_instr : call_sites) {
1044+
call_instr->eraseFromParent();
1045+
}
1046+
1047+
return call_sites.size();
1048+
}
1049+
10351050
int LlvmCodeGen::InlineConstFnAttrs(const FunctionContext::TypeDesc& ret_type,
10361051
const vector<FunctionContext::TypeDesc>& arg_types, llvm::Function* fn) {
10371052
int replaced = 0;

be/src/codegen/llvm-codegen.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,10 @@ class LlvmCodeGen {
410410
int ReplaceCallSitesWithValue(llvm::Function* caller, llvm::Value* replacement,
411411
const std::string& target_name);
412412

413+
/// Remove calls to functions with a name of 'target_name' from caller. The return value
414+
/// is the number of calls removed.
415+
int RemoveCallSites(llvm::Function* caller, const std::string& target_name);
416+
413417
/// This function replaces calls to FunctionContextImpl::GetConstFnAttr() with constants
414418
/// derived from 'return_type', 'arg_types' and the runtime state 'state_'. Please note
415419
/// that this function only replaces call instructions inside 'fn' so to replace the

be/src/exec/partitioned-hash-join-node-ir.cc

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ bool IR_ALWAYS_INLINE PartitionedHashJoinNode::ProcessProbeRowInnerJoin(
4949
TupleRow* matched_build_row = hash_tbl_iterator_.GetRow();
5050
DCHECK(matched_build_row != NULL);
5151

52+
ClearExprResultsPool(num_conjuncts, num_other_join_conjuncts);
53+
5254
// Create an output row with all probe/build tuples and evaluate the
5355
// non-equi-join conjuncts.
5456
CreateOutputRow(out_row, current_probe_row_, matched_build_row);
@@ -81,6 +83,8 @@ bool IR_ALWAYS_INLINE PartitionedHashJoinNode::ProcessProbeRowRightSemiJoins(
8183
DCHECK(matched_build_row != NULL);
8284

8385
if (hash_tbl_iterator_.IsMatched()) continue;
86+
ClearExprResultsPool(num_conjuncts, num_other_join_conjuncts);
87+
8488
// Evaluate the non-equi-join conjuncts against a temp row assembled from all
8589
// build and probe tuples.
8690
if (num_other_join_conjuncts > 0) {
@@ -120,6 +124,9 @@ bool IR_ALWAYS_INLINE PartitionedHashJoinNode::ProcessProbeRowLeftSemiJoins(
120124
for (; !hash_tbl_iterator_.AtEnd(); hash_tbl_iterator_.NextDuplicate()) {
121125
TupleRow* matched_build_row = hash_tbl_iterator_.GetRow();
122126
DCHECK(matched_build_row != NULL);
127+
128+
ClearExprResultsPool(num_conjuncts, num_other_join_conjuncts);
129+
123130
// Evaluate the non-equi-join conjuncts against a temp row assembled from all
124131
// build and probe tuples.
125132
if (num_other_join_conjuncts > 0) {
@@ -184,6 +191,9 @@ bool IR_ALWAYS_INLINE PartitionedHashJoinNode::ProcessProbeRowOuterJoins(
184191
for (; !hash_tbl_iterator_.AtEnd(); hash_tbl_iterator_.NextDuplicate()) {
185192
TupleRow* matched_build_row = hash_tbl_iterator_.GetRow();
186193
DCHECK(matched_build_row != NULL);
194+
195+
ClearExprResultsPool(num_conjuncts, num_other_join_conjuncts);
196+
187197
// Create an output row with all probe/build tuples and evaluate the
188198
// non-equi-join conjuncts.
189199
CreateOutputRow(out_row, current_probe_row_, matched_build_row);

be/src/exec/partitioned-hash-join-node.cc

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,12 @@
2020
#include <functional>
2121
#include <numeric>
2222
#include <sstream>
23+
#include <gflags/gflags.h>
2324
#include <gutil/strings/substitute.h>
2425

2526
#include "codegen/llvm-codegen.h"
27+
#include "common/logging.h"
28+
#include "gen-cpp/Metrics_types.h"
2629
#include "exec/blocking-join-node.inline.h"
2730
#include "exec/exec-node-util.h"
2831
#include "exec/exec-node.inline.h"
@@ -37,12 +40,18 @@
3740
#include "runtime/runtime-state.h"
3841
#include "util/cyclic-barrier.h"
3942
#include "util/debug-util.h"
43+
#include "util/pretty-printer.h"
4044
#include "util/runtime-profile-counters.h"
4145

4246
#include "gen-cpp/PlanNodes_types.h"
4347

4448
#include "common/names.h"
4549

50+
DEFINE_double_hidden(result_pool_mem_multiplier, 0.5, "");
51+
DEFINE_validator(result_pool_mem_multiplier, [](const char* flagname, double val) {
52+
return val > 0.0 && val <= 1.0;
53+
});
54+
4655
static const string PREPARE_FOR_READ_FAILED_ERROR_MSG =
4756
"Failed to acquire initial read buffer for stream in hash join node $0. Reducing "
4857
"query concurrency or increasing the memory limit may help this query to complete "
@@ -115,9 +124,13 @@ PartitionedHashJoinNode::PartitionedHashJoinNode(RuntimeState* state,
115124
probe_exprs_(pnode.probe_exprs_),
116125
other_join_conjuncts_(pnode.other_join_conjuncts_),
117126
hash_table_config_(*pnode.hash_table_config_),
127+
expr_results_mem_limit_(FLAGS_result_pool_mem_multiplier
128+
* state->query_mem_tracker()->GetLowestLimit(MemLimit::SOFT)),
118129
process_probe_batch_fn_(pnode.process_probe_batch_fn_),
119130
process_probe_batch_fn_level0_(pnode.process_probe_batch_fn_level0_) {
120131
memset(hash_tbls_, 0, sizeof(HashTable*) * PARTITION_FANOUT);
132+
VLOG(2) << "Conjunct expression results pool memory limit: "
133+
<< PrettyPrinter::Print(expr_results_mem_limit_, TUnit::BYTES);
121134
}
122135

123136
PartitionedHashJoinNode::~PartitionedHashJoinNode() {
@@ -1480,6 +1493,13 @@ Status PartitionedHashJoinPlanNode::CodegenProcessProbeBatch(
14801493
RETURN_IF_ERROR(ExecNode::CodegenEvalConjuncts(codegen, conjuncts_,
14811494
&eval_conjuncts_fn));
14821495

1496+
// Remove call to ClearExprResultsPool if no join conjuncts exist.
1497+
if (conjuncts_.empty() && other_join_conjuncts_.empty()) {
1498+
int removed =
1499+
codegen->RemoveCallSites(process_probe_batch_fn, "ClearExprResultsPool");
1500+
DCHECK_EQ(removed, 1);
1501+
}
1502+
14831503
// Replace all call sites with codegen version
14841504
int replaced = codegen->ReplaceCallSites(process_probe_batch_fn, eval_row_fn,
14851505
"EvalProbeRow");

be/src/exec/partitioned-hash-join-node.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -546,6 +546,11 @@ class PartitionedHashJoinNode : public BlockingJoinNode {
546546

547547
std::string NodeDebugString() const;
548548

549+
/// Checks if the expression results memory pool has exceeded the memory limit defined
550+
/// by 'expr_results_mem_limit_'. If it has, clears the mem pool.
551+
inline void ClearExprResultsPool(const int num_conjuncts,
552+
const int num_other_join_conjuncts);
553+
549554
RuntimeState* runtime_state_;
550555

551556
/// Our equi-join predicates "<lhs> = <rhs>" are separated into
@@ -562,6 +567,10 @@ class PartitionedHashJoinNode : public BlockingJoinNode {
562567
/// an instance of the HashTableCtx in Prepare(). Not Owned.
563568
const HashTableConfig& hash_table_config_;
564569

570+
/// Memory limit for the conjunct expression results pool used during building probe
571+
/// batches.
572+
const int64_t expr_results_mem_limit_;
573+
565574
/// Used for hash-related functionality, such as evaluating rows and calculating hashes.
566575
/// This owns the evaluators for the build and probe expressions used during insertion
567576
/// and probing of the hash tables.

be/src/exec/partitioned-hash-join-node.inline.h

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,22 @@ inline void PartitionedHashJoinNode::ResetForProbe() {
3232
ht_ctx_->expr_values_cache()->Reset();
3333
}
3434

35+
// Must not be inlined during codegen since calls to this function from ProcessProbeBatch
36+
// are removed by codegen if there are no conjuncts.
37+
//
38+
// The actual function calls are in the ProcessProbeRow* functions which are inlined into
39+
// ProcessProbeBatch, thus the codegen of ProcessProbeBatch will be able to eliminate the
40+
// calls to this function.
41+
inline void IR_NO_INLINE PartitionedHashJoinNode::ClearExprResultsPool(
42+
const int num_conjuncts, const int num_other_join_conjuncts) {
43+
DCHECK(expr_results_pool_.get() != nullptr);
44+
DCHECK(expr_results_mem_limit_ > 0);
45+
if (num_conjuncts + num_other_join_conjuncts > 0 &&
46+
UNLIKELY(expr_results_pool_->total_allocated_bytes() > expr_results_mem_limit_)) {
47+
expr_results_pool_->Clear();
48+
}
49+
}
50+
3551
}
3652

3753
#endif
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
# Licensed to the Apache Software Foundation (ASF) under one
2+
# or more contributor license agreements. See the NOTICE file
3+
# distributed with this work for additional information
4+
# regarding copyright ownership. The ASF licenses this file
5+
# to you under the Apache License, Version 2.0 (the
6+
# "License"); you may not use this file except in compliance
7+
# with the License. You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing,
12+
# software distributed under the License is distributed on an
13+
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
# KIND, either express or implied. See the License for the
15+
# specific language governing permissions and limitations
16+
# under the License.
17+
18+
from __future__ import absolute_import, division, print_function
19+
import pytest
20+
21+
from time import sleep
22+
23+
from tests.common.custom_cluster_test_suite import CustomClusterTestSuite
24+
from tests.common.impala_connection import RUNNING
25+
from tests.common.skip import SkipIfExploration
26+
from tests.util.retry import retry
27+
28+
29+
@SkipIfExploration.is_not_exhaustive()
30+
class TestConjuncts(CustomClusterTestSuite):
31+
32+
@pytest.mark.stress
33+
@pytest.mark.execute_serially
34+
@CustomClusterTestSuite.with_args(cluster_size=1)
35+
def test_create_cache_many_tables(self, unique_database):
36+
"""Replicates the situation behund IMPALA-14863 where processing a row batch can cause
37+
the memory usage of a fragment instance on an executor to consume up to the process
38+
memory limit. Must be a custom cluster test because the fragment instances from the
39+
bad query continues to run for a long time after the query is cancelled."""
40+
self.client.execute("CREATE TABLE {}.test_left_fact (product_sk BIGINT,"
41+
"drv_event_type STRING)".format(unique_database))
42+
self.client.execute("CREATE TABLE {}.test_right_cte (product_sk BIGINT,"
43+
"product_type STRING)".format(unique_database))
44+
45+
# Insert 1,024 rows into the Left Fact table (Exactly 1 Impala Batch). All rows share
46+
# the same join key (999).
47+
self.client.execute("""WITH ten AS (
48+
SELECT 0 AS n UNION ALL SELECT 1 UNION ALL SELECT 2 UNION ALL SELECT 3 UNION ALL
49+
SELECT 4 UNION ALL SELECT 5 UNION ALL SELECT 6 UNION ALL SELECT 7 UNION ALL
50+
SELECT 8 UNION ALL SELECT 9
51+
)
52+
INSERT INTO {}.test_left_fact
53+
SELECT 999 AS product_sk, 'fail_event' AS drv_event_type FROM ten a
54+
CROSS JOIN ten b CROSS JOIN ten c LIMIT 1024""".format(unique_database))
55+
56+
# Insert 1,000,000 rows into the Right CTE table. All 1 Million rows share the exact
57+
# same join key (999).
58+
self.client.execute("""WITH ten AS (
59+
SELECT 0 AS n UNION ALL SELECT 1 UNION ALL SELECT 2 UNION ALL SELECT 3 UNION ALL
60+
SELECT 4 UNION ALL SELECT 5 UNION ALL SELECT 6 UNION ALL SELECT 7 UNION ALL
61+
SELECT 8 UNION ALL SELECT 9
62+
)
63+
INSERT INTO {}.test_right_cte
64+
SELECT 999 AS product_sk, 'wrong_type' AS product_type FROM ten a
65+
CROSS JOIN ten b CROSS JOIN ten c CROSS JOIN ten d CROSS JOIN ten e
66+
CROSS JOIN ten f""".format(unique_database))
67+
68+
# Set a very low memory limit that will quickly be exceeded by the join conjuncts.
69+
self.client.set_configuration_option("mem_limit_executors", "185mb")
70+
71+
# Run a query that will cause the join conjuncts to exceed the memory limit.
72+
handle = self.client.execute_async("""SELECT STRAIGHT_JOIN count(1)
73+
FROM {0}.test_left_fact f
74+
LEFT JOIN {0}.test_right_cte p2
75+
ON f.product_sk = p2.product_sk
76+
WHERE (
77+
UPPER(f.drv_event_type) LIKE '%_CONF'
78+
OR
79+
(
80+
CASE
81+
WHEN UPPER(p2.product_type) = 'COMMODITY-SWAP'
82+
THEN UPPER(CONCAT(f.drv_event_type, '_SWAP_1'))
83+
WHEN UPPER(p2.product_type) = 'METAL-FUTURE'
84+
THEN UPPER(CONCAT(f.drv_event_type, '_METAL_2'))
85+
WHEN UPPER(p2.product_type) = 'ENERGY-FUTURE'
86+
THEN UPPER(CONCAT(f.drv_event_type, '_ENERGY_3'))
87+
WHEN UPPER(p2.product_type) = 'AGRICULTURAL-FUTURE'
88+
THEN UPPER(CONCAT(f.drv_event_type, '_AGRI_4'))
89+
WHEN UPPER(p2.product_type) = 'SOFT-FUTURE'
90+
THEN UPPER(CONCAT(f.drv_event_type, '_SOFT_5'))
91+
WHEN UPPER(p2.product_type) = 'OTHER-FUTURE'
92+
THEN UPPER(CONCAT(f.drv_event_type, '_OTHER_6'))
93+
WHEN UPPER(p2.product_type) = 'METAL-OPTION'
94+
THEN UPPER(CONCAT(f.drv_event_type, '_MOPT_7'))
95+
WHEN UPPER(p2.product_type) = 'ENERGY-OPTION'
96+
THEN UPPER(CONCAT(f.drv_event_type, '_EOPT_8'))
97+
WHEN UPPER(p2.product_type) = 'SPREAD-OPTION'
98+
THEN UPPER(CONCAT(f.drv_event_type, '_SOPT_9'))
99+
WHEN UPPER(p2.product_type) = 'COMMODITY-INDEX'
100+
THEN UPPER(CONCAT(f.drv_event_type, '_INDEX_10'))
101+
ELSE UPPER(CONCAT(UPPER(f.drv_event_type), '_UNKNOWN'))
102+
END = 'COMMODITY-FORWARD'
103+
)
104+
)""".format(unique_database))
105+
self.client.wait_for_impala_state(handle, RUNNING, 10)
106+
107+
# Wait for the query to consume more than 270mb of memory indicating it has started
108+
# evaluating the join conjuncts.
109+
def __wait_for_270mb():
110+
return self.get_metric("memory.total-used") > 270 * 1024 * 1024
111+
retry(__wait_for_270mb, 10, 1, 1)
112+
mem_before = self.get_metric("memory.total-used")
113+
114+
# Give the query some time to continue to run.
115+
sleep(10)
116+
117+
# Assert the memory usage did not increase more than 25mb during the sleep. If the
118+
# expression results pool is not being cleared properly, the memory usage will
119+
# continue to grow as more rows are processed.
120+
mem_after = self.get_metric("memory.total-used")
121+
assert mem_after - mem_before < 25 * 1024 * 1024, "Memory usage increased during " \
122+
"sleep. Before: {} After: {}".format(mem_before, mem_after)

0 commit comments

Comments
 (0)