Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12121 )
Change subject: [fs]: wrapping up containers in scoped_refptr ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/12121/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12121/3/src/kudu/fs/log_block_manager.cc@233 PS3, Line 233: LogBlockContainerRefPtr container_; This was my biggest concern with this change: in large deployments with millions of LogBlocks, the time spent incrementing container ref counts at startup and decrementing on tablet deletion could be substantial. Could you try to profile the effect of this change? If you have access to a large deployment, you could use the Linux perf tools to measure the impact on node startup before and after this change, to see if the ref count manipulation becomes a hot spot. http://gerrit.cloudera.org:8080/#/c/12121/3/src/kudu/fs/log_block_manager.cc@333 PS3, Line 333: // LogBlockContainer are reference counted to simplify support for deletion : // with LogBlock deletion. Nit: add an empty line just before and reword: Block containers are reference counted so that they can be safely removed despite concurrent access. http://gerrit.cloudera.org:8080/#/c/12121/3/src/kudu/fs/log_block_manager.cc@1248 PS3, Line 1248: // To keep using raw point as key in the unordered_map safely, : // it is important to save scoped_refptr of LogBlockContainer : // here. And, the reason why not saving scoped_refptr of LogBlock : // is that it will cause circular dependency issue. You could use LogBlockContainerRefPtr as a key to an associative container though. See ScopedRefPtrEqualToFunctor and ScopedRefPtrHashFunctor. http://gerrit.cloudera.org:8080/#/c/12121/3/src/kudu/fs/log_block_manager.cc@1584 PS3, Line 1584: // The owning container. Must outlive LogBlockRefPtr below. : LogBlockContainer* container_; Just drop this altogether. There are several good reasons to do that: - We'll save 8 bytes per block in memory. - It clarifies the lifecycle of the container vs. the LogBlock. - It makes the race on L1617 (between container_ and log_block_.reset()) obvious. -- To view, visit http://gerrit.cloudera.org:8080/12121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c5c620014782b3d57dcbe047d0df73c949590c7 Gerrit-Change-Number: 12121 Gerrit-PatchSet: 3 Gerrit-Owner: helifu <hzhel...@corp.netease.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <hzhel...@corp.netease.com> Gerrit-Comment-Date: Wed, 02 Jan 2019 23:18:14 +0000 Gerrit-HasComments: Yes
