You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ab...@apache.org on 2016/08/16 06:37:10 UTC
[1/2] incubator-impala git commit: IMPALA-1619,
IMPALA-3018: Address various small memory allocation related bugs
Repository: incubator-impala
Updated Branches:
refs/heads/master 3307acfef -> 3a630a5d6
IMPALA-1619, IMPALA-3018: Address various small memory allocation related bugs
This patch addresses a potential overflow in calculation FreePool::Rellocate()
and its handling of zero-length allocations. This patch also adds code to
gracefully handle malloc() failures when initializing/resizing hash tables.
Change-Id: I6eb9a4472a65cf68edb0323b13d745277ead2e1d
Reviewed-on: http://gerrit.cloudera.org:8080/3807
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Internal Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/5a55ba76
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/5a55ba76
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/5a55ba76
Branch: refs/heads/master
Commit: 5a55ba76108443fb7a136d6a48fb95840ab7e5e6
Parents: 3307acf
Author: Michael Ho <kw...@cloudera.com>
Authored: Wed Jul 27 12:22:43 2016 -0700
Committer: Internal Jenkins <cl...@gerrit.cloudera.org>
Committed: Tue Aug 16 04:27:11 2016 +0000
----------------------------------------------------------------------
be/src/exec/hash-table.cc | 10 +++++++++-
be/src/runtime/free-pool-test.cc | 5 +++++
be/src/runtime/free-pool.h | 4 ++--
3 files changed, 16 insertions(+), 3 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/5a55ba76/be/src/exec/hash-table.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/hash-table.cc b/be/src/exec/hash-table.cc
index 7c2eab1..f8df641 100644
--- a/be/src/exec/hash-table.cc
+++ b/be/src/exec/hash-table.cc
@@ -386,6 +386,11 @@ bool HashTable::Init() {
return false;
}
buckets_ = reinterpret_cast<Bucket*>(malloc(buckets_byte_size));
+ if (buckets_ == NULL) {
+ state_->block_mgr()->ReleaseMemory(block_mgr_client_, buckets_byte_size);
+ num_buckets_ = 0;
+ return false;
+ }
memset(buckets_, 0, buckets_byte_size);
return true;
}
@@ -438,7 +443,10 @@ bool HashTable::ResizeBuckets(int64_t num_buckets, const HashTableCtx* ht_ctx) {
int64_t new_size = num_buckets * sizeof(Bucket);
if (!state_->block_mgr()->ConsumeMemory(block_mgr_client_, new_size)) return false;
Bucket* new_buckets = reinterpret_cast<Bucket*>(malloc(new_size));
- DCHECK(new_buckets != NULL);
+ if (new_buckets == NULL) {
+ state_->block_mgr()->ReleaseMemory(block_mgr_client_, new_size);
+ return false;
+ }
memset(new_buckets, 0, new_size);
// Walk the old table and copy all the filled buckets to the new (resized) table.
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/5a55ba76/be/src/runtime/free-pool-test.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/free-pool-test.cc b/be/src/runtime/free-pool-test.cc
index d5eec25..1558683 100644
--- a/be/src/runtime/free-pool-test.cc
+++ b/be/src/runtime/free-pool-test.cc
@@ -187,6 +187,11 @@ TEST(FreePoolTest, ReAlloc) {
EXPECT_TRUE(ptr3 != ptr4);
EXPECT_EQ(mem_pool.total_allocated_bytes(), 1024 + 8 + 2048 + 8 + (1LL << 32) + 8);
+ // Shrink the allocation.
+ uint8_t* ptr5 = pool.Reallocate(ptr4, 1024);
+ EXPECT_TRUE(ptr4 == ptr5);
+ EXPECT_EQ(mem_pool.total_allocated_bytes(), 1024 + 8 + 2048 + 8 + (1LL << 32) + 8);
+
mem_pool.FreeAll();
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/5a55ba76/be/src/runtime/free-pool.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/free-pool.h b/be/src/runtime/free-pool.h
index a5fe8e6..f37f433 100644
--- a/be/src/runtime/free-pool.h
+++ b/be/src/runtime/free-pool.h
@@ -123,10 +123,10 @@ class FreePool {
return NULL;
}
#endif
+ if (UNLIKELY(ptr == NULL || ptr == mem_pool_->EmptyAllocPtr())) return Allocate(size);
if (FLAGS_disable_mem_pools) {
return reinterpret_cast<uint8_t*>(realloc(reinterpret_cast<void*>(ptr), size));
}
- if (UNLIKELY(ptr == NULL || ptr == mem_pool_->EmptyAllocPtr())) return Allocate(size);
FreeListNode* node = reinterpret_cast<FreeListNode*>(ptr - sizeof(FreeListNode));
FreeListNode* list = node->list;
#ifndef NDEBUG
@@ -135,7 +135,7 @@ class FreePool {
int bucket_idx = (list - &lists_[0]);
DCHECK_LT(bucket_idx, NUM_LISTS);
// This is the actual size of ptr.
- int allocation_size = 1 << bucket_idx;
+ int64_t allocation_size = 1LL << bucket_idx;
// If it's already big enough, just return the ptr.
if (allocation_size >= size) return ptr;
[2/2] incubator-impala git commit: IMPALA-3952: Clear scratch batch
mem pool if Open() failed.
Posted by ab...@apache.org.
IMPALA-3952: Clear scratch batch mem pool if Open() failed.
We used to be able to hit a DCHECK in HdfsParquetScanner::Close()
when using the legacy aggs/joins if HdfsParquetScanner::Open() failed.
With the legacy aggs/joins the tuple ptrs of the scratch batch are
allocated from the scratch batch's mem pool, and if Open() failed we
never freed or transferred the scratch batch's mem pool.
Testing: I tested this patch together with the fix for IMPALA-3964
on core/hdfs with the legacy aggs and joins enabled.
Change-Id: I55f32ed698a5b6fed8c28af1391aa07e1560e782
Reviewed-on: http://gerrit.cloudera.org:8080/3953
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Internal Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/3a630a5d
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/3a630a5d
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/3a630a5d
Branch: refs/heads/master
Commit: 3a630a5d6871db6afe9ca343aea80b14c6525f3d
Parents: 5a55ba7
Author: Alex Behm <al...@cloudera.com>
Authored: Thu Aug 11 21:30:01 2016 -0700
Committer: Internal Jenkins <cl...@gerrit.cloudera.org>
Committed: Tue Aug 16 06:08:50 2016 +0000
----------------------------------------------------------------------
be/src/exec/hdfs-parquet-scanner.cc | 7 +++++++
1 file changed, 7 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a630a5d/be/src/exec/hdfs-parquet-scanner.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/hdfs-parquet-scanner.cc b/be/src/exec/hdfs-parquet-scanner.cc
index b0fd008..1431689 100644
--- a/be/src/exec/hdfs-parquet-scanner.cc
+++ b/be/src/exec/hdfs-parquet-scanner.cc
@@ -50,6 +50,8 @@ using namespace impala;
DEFINE_double(parquet_min_filter_reject_ratio, 0.1, "(Advanced) If the percentage of "
"rows rejected by a runtime filter drops below this value, the filter is disabled.");
+DECLARE_bool(enable_partitioned_aggregation);
+DECLARE_bool(enable_partitioned_hash_join);
// The number of rows between checks to see if a filter is not effective, and should be
// disabled. Must be a power of two.
@@ -205,6 +207,11 @@ void HdfsParquetScanner::Close(RowBatch* row_batch) {
if (row_batch != NULL) {
FlushRowGroupResources(row_batch);
if (add_batches_to_queue_) scan_node_->AddMaterializedRowBatch(row_batch);
+ } else if (!FLAGS_enable_partitioned_hash_join ||
+ !FLAGS_enable_partitioned_aggregation) {
+ // With the legacy aggs/joins the tuple ptrs of the scratch batch are allocated
+ // from the scratch batch's mem pool. We can get into this case if Open() fails.
+ scratch_batch_->mem_pool()->FreeAll();
}
// Verify all resources (if any) have been transferred.
DCHECK_EQ(dictionary_pool_.get()->total_allocated_bytes(), 0);