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 <[email protected]> Reviewed-by: Dan Hecht <[email protected]> Tested-by: Jim Apple <[email protected]> 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/hadoop-next Commit: c68734ad6509869e8660cd1bad05e53461137474 Parents: b14f319 Author: Jim Apple <[email protected]> Authored: Wed Nov 9 14:22:20 2016 -0800 Committer: Jim Apple <[email protected]> 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; }
