Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9732 )
Change subject: striped64: reduce size from 192 to 16 bytes ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/9732/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9732/2//COMMIT_MSG@16 PS2, Line 16: - previously it used a virtual destructor, which required a vtbl to be : stored. This is a best practice but in this case we always delete : particular subclasses and don't hold on to references using the : superclass, so I think it's worth bending the rules a bit. Could you document this rule bending somewhere so that users of Striped64 are less likely to make that mistake? http://gerrit.cloudera.org:8080/#/c/9732/2/src/kudu/util/striped64.h File src/kudu/util/striped64.h: http://gerrit.cloudera.org:8080/#/c/9732/2/src/kudu/util/striped64.h@43 PS2, Line 43: AtomicInt<int64_t> value_; Replace this with std::atomic<int64_t>, for consistency? http://gerrit.cloudera.org:8080/#/c/9732/2/src/kudu/util/striped64.h@141 PS2, Line 141: class LongAdder : Striped64 { What do you think about simplifying further and removing the distinction between LongAdder and Striped64? I think the original motivation behind the separation was to eventually develop other counter types on top, but that clearly hasn't happened. By merging the two we could eliminate some of the complexity (such as the functor passed into RetryUpdate). -- To view, visit http://gerrit.cloudera.org:8080/9732 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9ba955357744935ac6242cd3b609412c7e9da66 Gerrit-Change-Number: 9732 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 05 Apr 2018 18:19:15 +0000 Gerrit-HasComments: Yes
