You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2019/11/27 17:02:14 UTC
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14804
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
IMPALA-9174: Speedup allocations of ORC Scanner
The ORC library provides a hook for its clients to plugin a custom
memory pool. This memory pool needs to override the 'malloc()' and
'free()' methods. Impala has its own memory pool named OrcMemPool.
Impala's OrcMemPool used to have an internal unordered_map to keep
track of its allocations. In 'free()' it used the map to lookup the
size of the allocated byte array. We need this information in 'free()'
because of memory tracking. Therefore for each 'malloc()' and 'free()'
there was an additional allocation/deallocation by the unordered_map.
Instead of using a map this patch allocates a few more bytes on each
allocation and store the size of the allocated bytes in front of the
allocated bytes, similarly to how the standard malloc/free works.
(Unfortunately the standard malloc/free doesn't reveal that information
in a standard way).
OrcMemPool also had a method called 'FreeAll()' which freed all
allocated memory. This was a no-op because the library only uses the
memory pool to allocate memory for the data buffers, and they free their
memory in their destructors. On the other hand, it provided some kind of
guard against memory leaks in the ORC library. We can add some checks to
the destructor of OrcMemPool to detect leaks if we don't trust the
library's memory management.
Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
---
M be/src/exec/hdfs-orc-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
2 files changed, 7 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/14804/1
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14804/6/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/6/be/src/exec/hdfs-orc-scanner.cc@65
PS6, Line 65: '1.5 * size'
nit: you don't always consume 1.5 * 'size' here according to the line below. Could you update the comment?
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jan 2020 14:26:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
I am ok with the implementation, but I think hdfs-orc-scanner.cc is not the ideal place for it, see my comment.
http://gerrit.cloudera.org:8080/#/c/14804/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14804/4//COMMIT_MSG@19
PS4, Line 19: malloc already stores the size of the allocated bytes in front of the
: allocated bytes. That's why 'free()' doesn't take a 'size' parameter,
: it just searches for the size information next to the pointed bytes.
: TC Malloc provides a function called 'tc_malloc_size()' that reveals
: that information programmatically, so instead of the hash table we
: can just use this function to retrieve the size.
This is not exactly how tc malloc stores sizes. There is an explanation in http://goog-perftools.sourceforge.net/doc/tcmalloc.html , mainly in "Small Object Allocation", "Spans", and Deallocation sections.
I would only mention that tc malloc has an API to retrieve the allocated size and provide link to the doc with details.
The main problem with always storing the size for every object is that in case of very small objects it leads to large overhead, so a common optimization in heap implementations is to store objects of similar size (class) in a contiguous area and only store the size once for that area. It also wouldn't be ideal for big allocations, where it makes sense to work only with K*4096 byte chunks.
http://gerrit.cloudera.org:8080/#/c/14804/4/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/4/be/src/exec/hdfs-orc-scanner.cc@63
PS4, Line 63: malloc
I think it would be better to move most of this functionality into MemTracker, e.g. to functions like TrackedMalloc + TrackedFree. This function could just call that and throw a suitable exception based on the result.
It would be also nice to create some tests that exercise most paths.
I am ok with creating a follow up jira for this and do it later.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Dec 2019 13:17:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14804
to look at the new patch set (#3).
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
IMPALA-9174: Speedup allocations of ORC Scanner
The ORC library provides a hook for its clients to plugin a custom
memory pool. This memory pool needs to override the 'malloc()' and
'free()' methods. Impala has its own memory pool named OrcMemPool.
Impala's OrcMemPool used to have an internal unordered_map to keep
track of its allocations. In 'free()' it used the map to lookup the
size of the allocated byte array. We need this information in 'free()'
because of memory tracking. Therefore for each 'malloc()' and 'free()'
there was an additional allocation/deallocation by the unordered_map.
malloc already stores the size of the allocated bytes in front of the
allocated bytes. That's why 'free()' doesn't take a 'size' parameter,
it just searches for the size information next to the pointed bytes.
TC Malloc provides a function called 'tc_malloc_size()' that reveals
that information programmatically, so instead of the hash table we
can just use this function to retrieve the size.
OrcMemPool also had a method called 'FreeAll()' which freed all
allocated memory. This was a no-op because the library only uses the
memory pool to allocate memory for the data buffers, and they free their
memory in their destructors. On the other hand, it provided some kind of
guard against memory leaks in the ORC library. We can add some checks to
the destructor of OrcMemPool to detect leaks if we don't trust the
library's memory management.
Performance
I ran single_node_perf_run.py to measure the performance gain:
TPCH scale 5:
+----------+-------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-------------------+---------+------------+------------+----------------+
| TPCH(5) | orc / def / block | 3.73 | -0.62% | 2.95 | -1.22% |
+----------+-------------------+---------+------------+------------+----------------+
(R) Regression: TPCH(5) TPCH-Q18 [orc / def / block] (5.22s -> 5.58s [+6.92%])
(I) Improvement: TPCH(5) TPCH-Q2 [orc / def / block] (1.62s -> 1.44s [-11.26%])
(I) Improvement: TPCH(5) TPCH-Q6 [orc / def / block] (1.33s -> 1.14s [-14.68%])
TPCH scale 10:
+----------+-------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-------------------+---------+------------+------------+----------------+
| TPCH(10) | orc / def / block | 6.57 | -0.64% | 4.93 | -0.94% |
+----------+-------------------+---------+------------+------------+----------------+
(I) Improvement: TPCH(10) TPCH-Q20 [orc / def / block] (5.94s -> 5.49s [-7.54%])
(I) Improvement: TPCH(10) TPCH-Q13 [orc / def / block] (5.33s -> 4.70s [-11.84%])
Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
---
M be/src/exec/hdfs-orc-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
2 files changed, 35 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/14804/3
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@74
PS1, Line 74: return addr + HEADER_SIZE;
> Thanks for noting it. Currently the ORC lib doesn't assume 16 byte alignmen
nitpicking: "it still uses less memory than the Base version" is not necessarily true - probably it is better for small allocations but can be worse for large ones. Adding even 1 byte to the allocations can move to a different size class, e.g. allocating 2049 instead of 2048 bytes will need 256 more bytes according to http://goog-perftools.sourceforge.net/doc/tcmalloc.html
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@80
PS1, Line 80: SizeType size = *reinterpret_cast<SizeType*>(p);
> Thanks for the pointer, although I'm not sure if it worth it, I'd rather ke
It doesn't matter AFAIK - size is rounder up to certain sizes (size classes), which will be the allocated size. So the original and allocated size should have the same size class, which is needed during free().
Found an issue that suggests that we don't win too much with using the size hint: https://github.com/gperftools/gperftools/issues/1096
Maybe a TODO can be added to give a hint if further optimization will be needed in the future.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Nov 2019 15:42:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 1:
(1 comment)
The solution makes sense to me. I originally was thinking that you could have used the malloc extensions (we have that dependency anyway), but I think that gets complicated because they don't return the exact size that you allocated.
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@67
PS1, Line 67: char* addr = static_cast<char*>(std::malloc(sizeof(SizeType) + size));
TCMalloc has an API to get the size of an allocation: https://github.com/gperftools/gperftools/blob/f47a52ce85c3d8d559aaae7b7a426c359fbca225/src/gperftools/tcmalloc.h.in#L118
There's also an equivalent for the sanitizer malloc - https://github.com/llvm-mirror/compiler-rt/blob/master/include/sanitizer/allocator_interface.h#L31
We already use both these different extensions, e.g. in be/src/util/memory-metrics.cc, so it's not a new dependency.
I guess the sizes are a little different because they include fragmentation overhead. Oh right, and we need to consume the memory before allocating anyway, so we don't know the allocated size when we call Consume(). So this idea probably doesn't work out.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 17:20:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 6: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5338/
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2019 16:35:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 1:
(4 comments)
Fixed the alignment issue. There are some open questions.
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@67
PS1, Line 67: char* addr = static_cast<char*>(std::malloc(sizeof(SizeType) + size));
> Yeah, this seems a hard question.
Yeah, maybe we could call Consume() first (maybe with some more bytes, like 'size * 1.5' or stg), then after allocation we could adjust the value in mem_tracker.
But I'm not sure if it worth the additional complexity.
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@74
PS1, Line 74: return addr + sizeof(SizeType);
> This could cause problems if the caller is assuming that this is more than
Thanks for noting it. Currently the ORC lib doesn't assume 16 byte alignment AFAICT. They have their own implementation of 128-bit integers: https://github.com/apache/orc/blob/branch-1.5/c%2B%2B/include/orc/Int128.hh
They use two 64-bit integers to represent a 128-bit integer.
Anyway, I added padding to keep it 16 byte aligned, it still uses less memory than the Base version.
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@78
PS1, Line 78: p -= sizeof(SizeType);
: SizeType size = *reinterpret_cast<SizeType*>(p);
> The orc library probably doesn't rely on this, but it is generally assumed
Thanks for catching this.
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@80
PS1, Line 80: std::free(p);
> Not sure if this worth the added dependency, but tc_malloc has a function c
Thanks for the pointer, although I'm not sure if it worth it, I'd rather keep it simple.
Btw does it need the actually allocated size, or the requested size?
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Nov 2019 14:34:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 7:
Based on my newest measurements it doesn't cause any perf improvement unfortunately. It also makes the code a bit less safe, since it doesn't free memory leaked by the ORC lib.
Therefore I'm gonna abandon this CR for now.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Feb 2020 08:20:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5160/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 17:46:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5231/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 17:32:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14804/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14804/4//COMMIT_MSG@19
PS4, Line 19: malloc already stores the size of the allocated bytes in front of the
: allocated bytes. That's why 'free()' doesn't take a 'size' parameter,
: it just searches for the size information next to the pointed bytes.
: TC Malloc provides a function called 'tc_malloc_size()' that reveals
: that information programmatically, so instead of the hash table we
: can just use this function to retrieve the size.
> This is not exactly how tc malloc stores sizes. There is an explanation in
Thanks for the clarification. I changed the wording.
http://gerrit.cloudera.org:8080/#/c/14804/4/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/4/be/src/exec/hdfs-orc-scanner.cc@63
PS4, Line 63: malloc
> I think it would be better to move most of this functionality into MemTrack
Currently MemTracker is only used to do the bookeeping of allocated memory, it doesn't allocate/release.
Objects of MemPool and ObjectPool are used for allocations, they have an internal MemTracker to check memory limits for TryAllocate*() functions. However, they cannot release arbitrary memory, they can only release all allocated memory at once.
I'd rather have this logic only here, since later we'll probably need to get rid of this OrcMemPool anyway, and switch to buffer-pool-based allocations.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Dec 2019 12:53:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14804/4/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/4/be/src/exec/hdfs-orc-scanner.cc@63
PS4, Line 63: malloc
> Currently MemTracker is only used to do the bookeeping of allocated memory,
I generally agree with Zoltan here. The other thing is that putting it in MemTracker might encourage people to use it as a convenient memory allocation function, which I don't think we want.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Dec 2019 17:08:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14804/3/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/3/be/src/exec/hdfs-orc-scanner.cc@81
PS3, Line 81: if (LIKELY(actual_alloc_size < overestimate_alloc_size)) {
> I think this won't compile with ASAN/TSAN
Oops, right. I added a new free function called 'GetAllocatedMemory()' to memory-metrics.h
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 17:04:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 2: Code-Review-1
I'm gonna try the malloc extensions and do some perf benchmarks.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 15:38:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 5: Code-Review+2
Carrying +2
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2019 12:27:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5170/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Nov 2019 15:18:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2019 12:27:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14804
to look at the new patch set (#4).
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
IMPALA-9174: Speedup allocations of ORC Scanner
The ORC library provides a hook for its clients to plugin a custom
memory pool. This memory pool needs to override the 'malloc()' and
'free()' methods. Impala has its own memory pool named OrcMemPool.
Impala's OrcMemPool used to have an internal unordered_map to keep
track of its allocations. In 'free()' it used the map to lookup the
size of the allocated byte array. We need this information in 'free()'
because of memory tracking. Therefore for each 'malloc()' and 'free()'
there was an additional allocation/deallocation by the unordered_map.
malloc already stores the size of the allocated bytes in front of the
allocated bytes. That's why 'free()' doesn't take a 'size' parameter,
it just searches for the size information next to the pointed bytes.
TC Malloc provides a function called 'tc_malloc_size()' that reveals
that information programmatically, so instead of the hash table we
can just use this function to retrieve the size.
OrcMemPool also had a method called 'FreeAll()' which freed all
allocated memory. This was a no-op because the library only uses the
memory pool to allocate memory for the data buffers, and they free their
memory in their destructors. On the other hand, it provided some kind of
guard against memory leaks in the ORC library. We can add some checks to
the destructor of OrcMemPool to detect leaks if we don't trust the
library's memory management.
Performance
I ran single_node_perf_run.py to measure the performance gain:
TPCH scale 5:
+----------+-------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-------------------+---------+------------+------------+----------------+
| TPCH(5) | orc / def / block | 3.73 | -0.62% | 2.95 | -1.22% |
+----------+-------------------+---------+------------+------------+----------------+
(R) Regression: TPCH(5) TPCH-Q18 [orc / def / block] (5.22s -> 5.58s [+6.92%])
(I) Improvement: TPCH(5) TPCH-Q2 [orc / def / block] (1.62s -> 1.44s [-11.26%])
(I) Improvement: TPCH(5) TPCH-Q6 [orc / def / block] (1.33s -> 1.14s [-14.68%])
TPCH scale 10:
+----------+-------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-------------------+---------+------------+------------+----------------+
| TPCH(10) | orc / def / block | 6.57 | -0.64% | 4.93 | -0.94% |
+----------+-------------------+---------+------------+------------+----------------+
(I) Improvement: TPCH(10) TPCH-Q20 [orc / def / block] (5.94s -> 5.49s [-7.54%])
(I) Improvement: TPCH(10) TPCH-Q13 [orc / def / block] (5.33s -> 4.70s [-11.84%])
Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
---
M be/src/exec/hdfs-orc-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/util/memory-metrics.h
3 files changed, 43 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/14804/4
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 3:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5221/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Dec 2019 15:20:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has abandoned this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 6:
My suspicion is that removing calling FreeAll() in the destructor led to leaking mem in some cases (e.g. if the query was cancelled at a specific point?).
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2019 18:08:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14804/2/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/2/be/src/exec/hdfs-orc-scanner.cc@63
PS2, Line 63: size
Curently we only consume 'size'. But maybe we should consume 'size + HEADER_SIZE'.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Nov 2019 14:37:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14804
to look at the new patch set (#2).
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
IMPALA-9174: Speedup allocations of ORC Scanner
The ORC library provides a hook for its clients to plugin a custom
memory pool. This memory pool needs to override the 'malloc()' and
'free()' methods. Impala has its own memory pool named OrcMemPool.
Impala's OrcMemPool used to have an internal unordered_map to keep
track of its allocations. In 'free()' it used the map to lookup the
size of the allocated byte array. We need this information in 'free()'
because of memory tracking. Therefore for each 'malloc()' and 'free()'
there was an additional allocation/deallocation by the unordered_map.
Instead of using a map this patch allocates a few more bytes on each
allocation and store the size of the allocated bytes in front of the
allocated bytes, similarly to how the standard malloc/free works.
(Unfortunately the standard malloc/free doesn't reveal that information
in a standard way).
OrcMemPool also had a method called 'FreeAll()' which freed all
allocated memory. This was a no-op because the library only uses the
memory pool to allocate memory for the data buffers, and they free their
memory in their destructors. On the other hand, it provided some kind of
guard against memory leaks in the ORC library. We can add some checks to
the destructor of OrcMemPool to detect leaks if we don't trust the
library's memory management.
Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
---
M be/src/exec/hdfs-orc-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
2 files changed, 11 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/14804/2
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5251/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Dec 2019 12:46:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@74
PS1, Line 74: return addr + sizeof(SizeType);
This could cause problems if the caller is assuming that this is more than 8-byte aligned. I think malloc() gives 16 byte alignment by default. I think it's unlikely that ORC is assuming 16 byte alignment but might be worth confirming (maybe they're using int128_t for decimals or similar - we had seen issues with some SIMD instructions on 16-byte decimals faulting within Impala because they did aligned loads - we had to force unaligned loads with memcpy() in those cases).
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 17:25:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
LGTM one you make the suggested adjustment for header size.
http://gerrit.cloudera.org:8080/#/c/14804/2/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/2/be/src/exec/hdfs-orc-scanner.cc@63
PS2, Line 63: size
> Curently we only consume 'size'. But maybe we should consume 'size + HEADER
That would make sense to me.
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 19:07:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5338/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2019 12:27:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 3:
(1 comment)
The approach makes sense, just one issue.
http://gerrit.cloudera.org:8080/#/c/14804/3/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/3/be/src/exec/hdfs-orc-scanner.cc@81
PS3, Line 81: uint64_t actual_alloc_size = tc_malloc_size(static_cast<void*>(addr));
I think this won't compile with ASAN/TSAN
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Dec 2019 17:17:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc
File be/src/exec/hdfs-orc-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@67
PS1, Line 67: char* addr = static_cast<char*>(std::malloc(sizeof(SizeType) + size));
> TCMalloc has an API to get the size of an allocation: https://github.com/gp
Yeah, this seems a hard question.
We could also TryConsume() the original size, then check the actual allocated size with tc_malloc, and then TryConsume() the difference again (and free + fail if that is not successful?).
This solution would be probably much slower, but could avoid the space overhead and the possible alignment issue.
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@78
PS1, Line 78: p -= sizeof(SizeType);
: SizeType size = *reinterpret_cast<SizeType*>(p);
The orc library probably doesn't rely on this, but it is generally assumed that calling free with nullptr is noop.
http://gerrit.cloudera.org:8080/#/c/14804/1/be/src/exec/hdfs-orc-scanner.cc@80
PS1, Line 80: std::free(p);
Not sure if this worth the added dependency, but tc_malloc has a function called tc_free_sized() which uses the size as a hint to speed up free: https://github.com/gperftools/gperftools/blob/f47a52ce85c3d8d559aaae7b7a426c359fbca225/src/gperftools/tcmalloc.h.in#L126
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Nov 2019 12:50:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14804 )
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
Patch Set 6:
looked into the failed job, there seems to be a memory leak in this query:
1348c2afc3e20d03:f5ddd10300000000] Analyzing query: select * from alltypes limit 101 db: functional_orc_def
this leads to a DCHECK:
F1213 14:35:27.704664 102182 exec-node.cc:178] 1348c2afc3e20d03:f5ddd10300000001] Check failed: mem_tracker()->consumption() == 0 (6554520 vs. 0) Leaked memory.
Fragment 1348c2afc3e20d03:f5ddd10300000001: Reservation=0 OtherMemory=6.25 MB Total=6.25 MB Peak=11.84 MB
HDFS_SCAN_NODE (id=0): Reservation=0 OtherMemory=6.25 MB Total=6.25 MB Peak=11.84 MB
KrpcDataStreamSender (dst_id=1): Total=0 Peak=1.55 KB
CodeGen: Total=0 Peak=0
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2019 17:20:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9174: Speedup allocations of ORC Scanner
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14804
to look at the new patch set (#5).
Change subject: IMPALA-9174: Speedup allocations of ORC Scanner
......................................................................
IMPALA-9174: Speedup allocations of ORC Scanner
The ORC library provides a hook for its clients to plugin a custom
memory pool. This memory pool needs to override the 'malloc()' and
'free()' methods. Impala has its own memory pool named OrcMemPool.
Impala's OrcMemPool used to have an internal unordered_map to keep
track of its allocations. In 'free()' it used the map to lookup the
size of the allocated byte array. We need this information in 'free()'
because of memory tracking. Therefore for each 'malloc()' and 'free()'
there was an additional allocation/deallocation by the unordered_map.
malloc implementations already store the size of the allocated bytes.
That's why 'free()' doesn't take a 'size' parameter, it just searches
for the size information.
TC Malloc provides a function called 'tc_malloc_size()' that reveals
that information programmatically, so instead of the hash table we
can just use this function to retrieve the size.
OrcMemPool also had a method called 'FreeAll()' which freed all
allocated memory. This was a no-op because the library only uses the
memory pool to allocate memory for the data buffers, and they free their
memory in their destructors. On the other hand, it provided some kind of
guard against memory leaks in the ORC library. We can add some checks to
the destructor of OrcMemPool to detect leaks if we don't trust the
library's memory management.
Performance
I ran single_node_perf_run.py to measure the performance gain:
TPCH scale 5:
+----------+-------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-------------------+---------+------------+------------+----------------+
| TPCH(5) | orc / def / block | 3.73 | -0.62% | 2.95 | -1.22% |
+----------+-------------------+---------+------------+------------+----------------+
(R) Regression: TPCH(5) TPCH-Q18 [orc / def / block] (5.22s -> 5.58s [+6.92%])
(I) Improvement: TPCH(5) TPCH-Q2 [orc / def / block] (1.62s -> 1.44s [-11.26%])
(I) Improvement: TPCH(5) TPCH-Q6 [orc / def / block] (1.33s -> 1.14s [-14.68%])
TPCH scale 10:
+----------+-------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-------------------+---------+------------+------------+----------------+
| TPCH(10) | orc / def / block | 6.57 | -0.64% | 4.93 | -0.94% |
+----------+-------------------+---------+------------+------------+----------------+
(I) Improvement: TPCH(10) TPCH-Q20 [orc / def / block] (5.94s -> 5.49s [-7.54%])
(I) Improvement: TPCH(10) TPCH-Q13 [orc / def / block] (5.33s -> 4.70s [-11.84%])
Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
---
M be/src/exec/hdfs-orc-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/util/memory-metrics.h
3 files changed, 43 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/14804/5
--
To view, visit http://gerrit.cloudera.org:8080/14804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia09e746883176d6f955c1718267bf55e2abb239b
Gerrit-Change-Number: 14804
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>