You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jb...@apache.org on 2016/11/11 03:45:42 UTC
[2/2] incubator-impala git commit: IMPALA-4455:
MemPoolTest.TryAllocateAligned failure: sizeof v. alignof
IMPALA-4455: MemPoolTest.TryAllocateAligned failure: sizeof v. alignof
This was testing all memory alignments up to and including
sizeof(max_align_t), but the standard says nothing about that. It does
say things about alignof(max_align_t), including that malloc() returns
memory at least that aligned.
In both gcc and clang on our currently supported platforms,
max_align_t has sizeof == 32 and alignof == 16, so this test expected
an alignment that malloc was not guaranteed to provide.
Change-Id: Ic2dbabcb9af2874d8ed354738243dfca9c492b08
Reviewed-on: http://gerrit.cloudera.org:8080/5022
Reviewed-by: Henry Robinson <he...@cloudera.com>
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Jim Apple <jb...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/c68734ad
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/c68734ad
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/c68734ad
Branch: refs/heads/master
Commit: c68734ad6509869e8660cd1bad05e53461137474
Parents: b14f319
Author: Jim Apple <jb...@cloudera.com>
Authored: Wed Nov 9 14:22:20 2016 -0800
Committer: Jim Apple <jb...@cloudera.com>
Committed: Fri Nov 11 03:44:07 2016 +0000
----------------------------------------------------------------------
be/src/exprs/anyval-util.cc | 9 ++++-----
be/src/exprs/anyval-util.h | 22 ++++++++++++++++++++++
be/src/runtime/mem-pool-test.cc | 2 +-
be/src/runtime/mem-pool.cc | 2 +-
be/src/runtime/mem-pool.h | 12 ++++++------
5 files changed, 34 insertions(+), 13 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c68734ad/be/src/exprs/anyval-util.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/anyval-util.cc b/be/src/exprs/anyval-util.cc
index e4d9337..132d6e4 100644
--- a/be/src/exprs/anyval-util.cc
+++ b/be/src/exprs/anyval-util.cc
@@ -31,11 +31,10 @@ namespace impala {
Status AllocateAnyVal(RuntimeState* state, MemPool* pool, const ColumnType& type,
const std::string& mem_limit_exceeded_msg, AnyVal** result) {
- int anyval_size = AnyValUtil::AnyValSize(type);
- // Ensure the allocation is sufficiently aligned (e.g. DecimalVal has a 16-byte
- // alignment requirement).
- int alignment = BitUtil::RoundUpToPowerOfTwo(anyval_size);
- *result = reinterpret_cast<AnyVal*>(pool->TryAllocateAligned(anyval_size, alignment));
+ const int anyval_size = AnyValUtil::AnyValSize(type);
+ const int anyval_alignment = AnyValUtil::AnyValAlignment(type);
+ *result =
+ reinterpret_cast<AnyVal*>(pool->TryAllocateAligned(anyval_size, anyval_alignment));
if (*result == NULL) {
return pool->mem_tracker()->MemLimitExceeded(
state, mem_limit_exceeded_msg, anyval_size);
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c68734ad/be/src/exprs/anyval-util.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/anyval-util.h b/be/src/exprs/anyval-util.h
index b84187a..8c75247 100644
--- a/be/src/exprs/anyval-util.h
+++ b/be/src/exprs/anyval-util.h
@@ -179,6 +179,28 @@ class AnyValUtil {
}
}
+ /// Returns the byte alignment of *Val for type t.
+ static int AnyValAlignment(const ColumnType& t) {
+ switch (t.type) {
+ case TYPE_BOOLEAN: return alignof(BooleanVal);
+ case TYPE_TINYINT: return alignof(TinyIntVal);
+ case TYPE_SMALLINT: return alignof(SmallIntVal);
+ case TYPE_INT: return alignof(IntVal);
+ case TYPE_BIGINT: return alignof(BigIntVal);
+ case TYPE_FLOAT: return alignof(FloatVal);
+ case TYPE_DOUBLE: return alignof(DoubleVal);
+ case TYPE_STRING:
+ case TYPE_VARCHAR:
+ case TYPE_CHAR:
+ return alignof(StringVal);
+ case TYPE_TIMESTAMP: return alignof(TimestampVal);
+ case TYPE_DECIMAL: return alignof(DecimalVal);
+ default:
+ DCHECK(false) << t;
+ return 0;
+ }
+ }
+
static std::string ToString(const StringVal& v) {
return std::string(reinterpret_cast<char*>(v.ptr), v.len);
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c68734ad/be/src/runtime/mem-pool-test.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/mem-pool-test.cc b/be/src/runtime/mem-pool-test.cc
index 70fb141..e6cb3b9 100644
--- a/be/src/runtime/mem-pool-test.cc
+++ b/be/src/runtime/mem-pool-test.cc
@@ -506,7 +506,7 @@ TEST(MemPoolTest, TryAllocateAligned) {
uint8_t* ptr = pool.TryAllocateAligned(size, alignment);
ASSERT_TRUE(ptr != NULL);
ASSERT_EQ(0, reinterpret_cast<uintptr_t>(ptr) % alignment);
- alignment = alignment == sizeof(std::max_align_t) ? 1 : alignment * 2;
+ alignment = alignment == alignof(std::max_align_t) ? 1 : alignment * 2;
size = (size + 1) % MAX_ALLOCATION_SIZE;
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c68734ad/be/src/runtime/mem-pool.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/mem-pool.cc b/be/src/runtime/mem-pool.cc
index c95953a..5f3feca 100644
--- a/be/src/runtime/mem-pool.cc
+++ b/be/src/runtime/mem-pool.cc
@@ -133,7 +133,7 @@ bool MemPool::FindChunk(int64_t min_size, bool check_limits) noexcept {
if (FLAGS_disable_mem_pools) {
// Disable pooling by sizing the chunk to fit only this allocation.
// Make sure the alignment guarantees are respected.
- chunk_size = std::max<int64_t>(min_size, sizeof(std::max_align_t));
+ chunk_size = std::max<int64_t>(min_size, alignof(std::max_align_t));
} else {
DCHECK_GE(next_chunk_size_, INITIAL_CHUNK_SIZE);
chunk_size = max<int64_t>(min_size, next_chunk_size_);
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c68734ad/be/src/runtime/mem-pool.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/mem-pool.h b/be/src/runtime/mem-pool.h
index 8980d22..b0fa694 100644
--- a/be/src/runtime/mem-pool.h
+++ b/be/src/runtime/mem-pool.h
@@ -111,10 +111,10 @@ class MemPool {
}
/// Same as TryAllocate() except a non-default alignment can be specified. It
- /// should be a power-of-two in [1, sizeof(std::max_align_t)].
+ /// should be a power-of-two in [1, alignof(std::max_align_t)].
uint8_t* TryAllocateAligned(int64_t size, int alignment) noexcept {
DCHECK_GE(alignment, 1);
- DCHECK_LE(alignment, sizeof(std::max_align_t));
+ DCHECK_LE(alignment, alignof(std::max_align_t));
DCHECK_EQ(BitUtil::RoundUpToPowerOfTwo(alignment), alignment);
return Allocate<true>(size, alignment);
}
@@ -255,11 +255,11 @@ class MemPool {
}
if (!fits_in_chunk) {
- // If we couldn't allocate a new chunk, return NULL.
- // malloc() guarantees alignment of min(std::max_align_t, min_chunk_size),
- // so we do not need to do anything additional to guarantee alignment.
+ // If we couldn't allocate a new chunk, return NULL. malloc() guarantees alignment
+ // of alignof(std::max_align_t), so we do not need to do anything additional to
+ // guarantee alignment.
static_assert(
- INITIAL_CHUNK_SIZE >= sizeof(std::max_align_t), "Min chunk size too low");
+ INITIAL_CHUNK_SIZE >= alignof(std::max_align_t), "Min chunk size too low");
if (UNLIKELY(!FindChunk(size, CHECK_LIMIT_FIRST))) return NULL;
}