Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15802 )
Change subject: KUDU-2844 (3/3): avoid copying plain/dict strings to RowBlock Arena ...................................................................... Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/15802/6/src/kudu/cfile/binary_dict_block.cc File src/kudu/cfile/binary_dict_block.cc: http://gerrit.cloudera.org:8080/#/c/15802/6/src/kudu/cfile/binary_dict_block.cc@298 PS6, Line 298: dst->memory()->RetainReference(dict_decoder_->block_handle()); Would it make sense to only retain the reference after we've verified there are results being pointed to? http://gerrit.cloudera.org:8080/#/c/15802/6/src/kudu/cfile/binary_dict_block.cc@313 PS6, Line 313: copy data to block. nit: could you update this to reflect that we're just pointing slices at the now-anchored blocks? Same below. http://gerrit.cloudera.org:8080/#/c/15802/3/src/kudu/common/rowblock_memory.h File src/kudu/common/rowblock_memory.h: http://gerrit.cloudera.org:8080/#/c/15802/3/src/kudu/common/rowblock_memory.h@46 PS3, Line 46: : void Reset() { > this code changed a bit with the switch to shared_ptr, take a look and see Ah got it. I looked around a bit in hopes that there was some interface along the lines of RefCountedThreadSafeBase we could use to Release() agnostic of the data type instead of taking function pointers, but I don't think that's tenable without some significant changes to RefCountedThreadSafe. -- To view, visit http://gerrit.cloudera.org:8080/15802 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I93fa1f9fd401814a42dc5a1f3fd2ffb1286ac441 Gerrit-Change-Number: 15802 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Thu, 13 Aug 2020 23:14:33 +0000 Gerrit-HasComments: Yes
