impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()
Date Wed, 04 Oct 2017 19:41:58 GMT
Tim Armstrong has posted comments on this change. ( )

Change subject: IMPALA-5988: optimise MemPool::TryAllocate()

Patch Set 3:

File be/src/runtime/mem-pool.h:
PS3, Line 122:   uint8_t* TryAllocateUnaligned(int64_t size) noexcept {
> Change lgtm. Mind explaining the original motivation behind the 8-byte defa
The default has been 8 byte aligned for as long as I'm aware. 
I added the alignment parameter because there was a case where we needed 16-byte alignment

I think 8-byte alignment is a reasonable default and matches what malloc() does. There's sometimes
a perf benefit (although it's pretty limited on recent Intel processors). It might make sense
to use unaligned memory in some other cases but it'd probably need perf validation.
PS3, Line 123: Allocate<true>(size, 1);
> TryAllocateAligned(size, 1);
This was somewhat deliberate - I wanted to force inlining into this function so that the alignment
logic could be optimised out. Added a comment - lmk if that addresses the comment.

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
Gerrit-Change-Number: 8145
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Wed, 04 Oct 2017 19:41:58 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message