[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8145 )

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

IMPALA-5988: optimise MemPool::TryAllocate()

Testing:
Ran core tests.

Perf:
Experiments using this on top of a WIP Avro patch for IMPALA-5307
showed noticable improvements in CPU efficiency - up to 10%

Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
Reviewed-on: http://gerrit.cloudera.org:8080/8145
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/exec/analytic-eval-node.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/text-converter.inline.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/runtime/mem-pool.h
6 files changed, 32 insertions(+), 21 deletions(-)

Approvals:
  Tim Armstrong: Looks good to me, approved
  Impala Public Jenkins: Verified

--
To view, visit http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
Gerrit-Change-Number: 8145
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8145 )

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


Patch Set 5: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
Gerrit-Change-Number: 8145
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 05 Oct 2017 02:50:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8145 )

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


Patch Set 5: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
Gerrit-Change-Number: 8145
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 04 Oct 2017 22:17:01 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8145 )

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


Patch Set 4: Code-Review+2

carry


--
To view, visit http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
Gerrit-Change-Number: 8145
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 04 Oct 2017 22:16:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8145 )

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


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1300/


--
To view, visit http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
Gerrit-Change-Number: 8145
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 04 Oct 2017 22:17:10 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8145 )

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


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8145/3/be/src/runtime/mem-pool.h
File be/src/runtime/mem-pool.h:

http://gerrit.cloudera.org:8080/#/c/8145/3/be/src/runtime/mem-pool.h@122
PS3, Line 122:   uint8_t* TryAllocateUnaligned(int64_t size) noexcept {
> The default has been 8 byte aligned for as long as I'm aware.
Thanks, works for me. We can still reconsider the default later if necessary. 
We do make many small allocations in other places like CollectionValueBuilder 
(for collections with small tuples).



--
To view, visit http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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 21:15:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8145 )

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


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8145/3/be/src/runtime/mem-pool.h
File be/src/runtime/mem-pool.h:

http://gerrit.cloudera.org:8080/#/c/8145/3/be/src/runtime/mem-pool.h@122
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.


http://gerrit.cloudera.org:8080/#/c/8145/3/be/src/runtime/mem-pool.h@123
PS3, Line 123: Allocate(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 http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Tim Armstrong (Code Review)]

Hello Alex Behm, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8145

to look at the new patch set (#4).

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

IMPALA-5988: optimise MemPool::TryAllocate()

Testing:
Ran core tests.

Perf:
Experiments using this on top of a WIP Avro patch for IMPALA-5307
showed noticable improvements in CPU efficiency - up to 10%

Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
---
M be/src/exec/analytic-eval-node.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/text-converter.inline.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/runtime/mem-pool.h
6 files changed, 32 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/8145/4
--
To view, visit http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
Gerrit-Change-Number: 8145
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8145 )

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


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8145/3/be/src/runtime/mem-pool.h
File be/src/runtime/mem-pool.h:

http://gerrit.cloudera.org:8080/#/c/8145/3/be/src/runtime/mem-pool.h@123
PS3, Line 123: Allocate(size, 1);
TryAllocateAligned(size, 1);
seems a bit clearer to me (then it's clearer this is just syntactic sugar over 
the core API).



--
To view, visit http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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-Comment-Date: Wed, 04 Oct 2017 17:18:46 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8145 )

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


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8145/3/be/src/runtime/mem-pool.h
File be/src/runtime/mem-pool.h:

http://gerrit.cloudera.org:8080/#/c/8145/3/be/src/runtime/mem-pool.h@122
PS3, Line 122:   uint8_t* TryAllocateUnaligned(int64_t size) noexcept {
Change lgtm. Mind explaining the original motivation behind the 8-byte default 
alignment? Looking at the callers of TryAllocate() I wonder why the default 
should not be unaligned.



--
To view, visit http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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-Comment-Date: Tue, 03 Oct 2017 22:12:50 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5988: optimise MemPool::TryAllocate()

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8145 )

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

IMPALA-5988: optimise MemPool::TryAllocate()

Testing:
Ran core tests.

Perf:
Experiments using this on top of a WIP Avro patch for IMPALA-5307
showed noticable improvements in CPU efficiency - up to 10%

Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
---
M be/src/exec/analytic-eval-node.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/text-converter.inline.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/runtime/mem-pool.h
6 files changed, 30 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/8145/2
--
To view, visit http://gerrit.cloudera.org:8080/8145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I088012084fe535a67a3cd1103ced5a02027f961a
Gerrit-Change-Number: 8145
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong