Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18053 )
Change subject: [bootstrap] Speedup tablet bootstrap ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/18053/4//COMMIT_MSG Commit Message: PS4: > Can you add some benchmarks before and after using a release build to the c +1 http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/fs/fs_manager.cc@719 PS4, Line 719: push_back(child) nit: could use the move semantics for 'child', moving strings from one vector to another to avoid extra memory allocations http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/tablet/tablet_bootstrap.cc@654 PS4, Line 654: and nit: the prior version was grammatically correct and easy to understand English, changing 'or' to 'and' here brings more confusion than clarity. If you want to clarify, you could change to ... neither blocks nor log segments found ... http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/tablet/tablet_bootstrap.cc@826 PS4, Line 826: return b.term() != a.term() || b.index() <= a.index() + 1; I don't think this makes the code clearer. As for the optimizations: wouldn't the compiler be able to do the right thing itself? I'm curious: did you compare the assembly to justify sacrificing the readability for any non-negligible performance gain? http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/tserver/ts_tablet_manager.cc@438 PS4, Line 438: nit: an extra line http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/tserver/ts_tablet_manager.cc@489 PS4, Line 489: seen_fatal_error What's difference between a fatal and a non-fatal one? Please add a comment to clarify or just rename 'xxx_fatal_error' --> 'xxx_error' below http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/tserver/ts_tablet_manager.cc@494 PS4, Line 494: seen_fatal_error.load() nit here and below: for STL atomics, it's not necessary to use load() or other explicit methods -- the implicit conversion operators are supposed to do the right thing http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/tserver/ts_tablet_manager.cc@519 PS4, Line 519: Missed '$0'? http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/tserver/ts_tablet_manager.cc@524 PS4, Line 524: std::lock_guard<simple_spinlock> l(lock); Using atomics everywhere and then resorting to serialized/locking access looks a bit strange. Instead, what if allocate 'metas' with empty/null elements and pass the index/pointer to the particular element to each thread so it could replace a dedicated element its particular meta? http://gerrit.cloudera.org:8080/#/c/18053/4/src/kudu/tserver/ts_tablet_manager.cc@1416 PS4, Line 1416: // TODO(yingchun): use macro to simplify code nit: this comment is too vague -- I'm not sure anybody (even you yourself) in a few weeks from now will be able to decipher this; so please either add more details for the prescribed action or remove this comment -- To view, visit http://gerrit.cloudera.org:8080/18053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I816417d2d4c24014edb6b2a40c060f29e37ae219 Gerrit-Change-Number: 18053 Gerrit-PatchSet: 4 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 01 Dec 2021 03:04:53 +0000 Gerrit-HasComments: Yes
