Adar Lieber-Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/16777 )
Change subject: KUDU-3108: fix invalid memory accesses in merge iterator ...................................................................... Patch Set 7: Code-Review+2 (4 comments) http://gerrit.cloudera.org:8080/#/c/16777/6/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: http://gerrit.cloudera.org:8080/#/c/16777/6/src/kudu/common/generic_iterators.cc@556 PS6, Line 556: bool operator()(const MergeIterState* a, const MergeIterState* b) const { > I tried getting this working with boost::intrusive::set<>, but ran into > issues that I don't fully understand yet (segfault in find()). The > implementation also isn't pretty, since we are using boost::intrusive on > pointers rather than objects. Regardless, I don't think it would circumvent > the extra call find(), so I'm unsure of how much faster this will actually > make things. >From an algorithmic standpoint, it should be comparable to std::set, but >intrusive::set doesn't do any memory allocation, so you wouldn't see that >overhead. But since you managed to get equal perf with two heaps (or even better perf than with three?), that's a very good place to pause and merge the fix as-is. http://gerrit.cloudera.org:8080/#/c/16777/7/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: http://gerrit.cloudera.org:8080/#/c/16777/7/src/kudu/common/generic_iterators.cc@565 PS7, Line 565: // The min-heaps and ordered set as described in the algorithm above. No ordered set anymore? http://gerrit.cloudera.org:8080/#/c/16777/7/src/kudu/common/generic_iterators.cc@677 PS7, Line 677: bool pulled_new_block = false; Shouldn't be necessary: if you don't return from Advance, pulled_new_block is guaranteed to be set. http://gerrit.cloudera.org:8080/#/c/16777/7/src/kudu/common/generic_iterators.cc@884 PS7, Line 884: // Since we're advancing iterators of the same row starting with hot_.top(), This is an interesting, but is it relevant to understanding MaterializeOneRow? -- To view, visit http://gerrit.cloudera.org:8080/16777 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ec1cd3fd67ec4ea92a55b5b0ce555123748824d Gerrit-Change-Number: 16777 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Lieber-Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Sat, 28 Nov 2020 05:33:24 +0000 Gerrit-HasComments: Yes
