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 <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 28 Nov 2019 14:34:55 +0000 Gerrit-HasComments: Yes
