Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] improved performance of BlockingQueue ...................................................................... Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/16063/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16063/5//COMMIT_MSG@20 PS5, Line 20: Before this patch: Nice improvement! I'm curious: did you get a sense for which of the deque vs move improvements was the major contributing factor? They seem to be in the same spirit of reducing unfavorable heap allocations at least. http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/consensus/log.cc@845 PS5, Line 845: entry_batch_raw If we've already transferred ownership of the unique_ptr to the blocking queue, won't the failed BlockingPut() call have already destructed this? Oh, though I suppose this is only used for identification of the trace event. Probably worth a comment acknowledging it, since upon first glance it might look like we're using state we shouldn't be. http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/util/blocking_queue.h File src/kudu/util/blocking_queue.h: http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/util/blocking_queue.h@145 PS5, Line 145: #if 0 Remove? -- To view, visit http://gerrit.cloudera.org:8080/16063 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2 Gerrit-Change-Number: 16063 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin <[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: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 12 Jun 2020 03:21:17 +0000 Gerrit-HasComments: Yes
