[kudu-CR] threadpool: simplify Submit API
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15401 ) Change subject: threadpool: simplify Submit API .. threadpool: simplify Submit API In an effort to modernize our codebase, I'm working to reduce the usage of boost::bind and std::bind. They can be actively harmful[1], and by comparison, lambdas offer the compiler more opportunities to inline. Also, std::bind tends to be aggressive with using allocations for storage, though boost::bind is better about this. This patch modifies ThreadPool to expose just a single Submit API which expects an std::function. Furthermore, all callers have been converted away from bind-based function creation and towards lambdas, which implicitly cast to std::function. There are contortions in a couple places to accommodate the lack of C++14 lambda support (e.g. we need capture-by-move semantics in RaftConsensus), but these are minimal. 1. https://abseil.io/tips/108 Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Reviewed-on: http://gerrit.cloudera.org:8080/15401 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin Reviewed-by: Andrew Wong --- M src/kudu/codegen/compilation_manager.cc M src/kudu/consensus/consensus-test-util.h M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/fs/dir_manager.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/rpc/reactor.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/thrift/client.h M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_checksum.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/table_scanner.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/util/countdown_latch-test.cc M src/kudu/util/curl_util-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/net/dns_resolver.cc M src/kudu/util/net/dns_resolver.h M src/kudu/util/threadpool-test.cc M src/kudu/util/threadpool.cc M src/kudu/util/threadpool.h 27 files changed, 264 insertions(+), 360 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] threadpool: simplify Submit API
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15401 ) Change subject: threadpool: simplify Submit API .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 12 Mar 2020 23:27:17 + Gerrit-HasComments: No
[kudu-CR] threadpool: simplify Submit API
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15401 ) Change subject: threadpool: simplify Submit API .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/15401/4/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: http://gerrit.cloudera.org:8080/#/c/15401/4/src/kudu/util/threadpool-test.cc@475 PS4, Line 475: > Yeah I think I'm gonna punt. It's test-only so not really seeing the value SGTM -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 12 Mar 2020 23:07:55 + Gerrit-HasComments: Yes
[kudu-CR] threadpool: simplify Submit API
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15401 to look at the new patch set (#5). Change subject: threadpool: simplify Submit API .. threadpool: simplify Submit API In an effort to modernize our codebase, I'm working to reduce the usage of boost::bind and std::bind. They can be actively harmful[1], and by comparison, lambdas offer the compiler more opportunities to inline. Also, std::bind tends to be aggressive with using allocations for storage, though boost::bind is better about this. This patch modifies ThreadPool to expose just a single Submit API which expects an std::function. Furthermore, all callers have been converted away from bind-based function creation and towards lambdas, which implicitly cast to std::function. There are contortions in a couple places to accommodate the lack of C++14 lambda support (e.g. we need capture-by-move semantics in RaftConsensus), but these are minimal. 1. https://abseil.io/tips/108 Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d --- M src/kudu/codegen/compilation_manager.cc M src/kudu/consensus/consensus-test-util.h M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/fs/dir_manager.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/rpc/reactor.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/thrift/client.h M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_checksum.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/table_scanner.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/util/countdown_latch-test.cc M src/kudu/util/curl_util-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/net/dns_resolver.cc M src/kudu/util/net/dns_resolver.h M src/kudu/util/threadpool-test.cc M src/kudu/util/threadpool.cc M src/kudu/util/threadpool.h 27 files changed, 264 insertions(+), 360 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15401/5 -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] threadpool: simplify Submit API
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15401 ) Change subject: threadpool: simplify Submit API .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/15401/4/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: http://gerrit.cloudera.org:8080/#/c/15401/4/src/kudu/util/threadpool-test.cc@475 PS4, Line 475: > nit: once dropping virtual for the destructor, maybe add final specifier fo Yeah I think I'm gonna punt. It's test-only so not really seeing the value in ensuring people don't misuse it. -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 12 Mar 2020 22:22:53 + Gerrit-HasComments: Yes
[kudu-CR] threadpool: simplify Submit API
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/15401 )
Change subject: threadpool: simplify Submit API
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15401/4/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
http://gerrit.cloudera.org:8080/#/c/15401/4/src/kudu/util/threadpool-test.cc@475
PS4, Line 475: class SlowDestructorRunnable {
nit: once dropping virtual for the destructor, maybe add final specifier for
the class?
https://en.cppreference.com/w/cpp/language/final
Feel free to ignore.
--
To view, visit http://gerrit.cloudera.org:8080/15401
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d
Gerrit-Change-Number: 15401
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 12 Mar 2020 01:32:45 +
Gerrit-HasComments: Yes
[kudu-CR] threadpool: simplify Submit API
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15401 ) Change subject: threadpool: simplify Submit API .. Patch Set 4: This time IWYU is not happy: >>> Fixing #includes in >>> '/home/jenkins-slave/workspace/kudu-master/1/src/kudu/util/threadpool-test.cc' @@ -28,6 +28,7 @@ #include #include #include +#include #include #include IWYU would have edited 1 files on your behalf. -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 12 Mar 2020 01:26:47 + Gerrit-HasComments: No
[kudu-CR] threadpool: simplify Submit API
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15401 to look at the new patch set (#4). Change subject: threadpool: simplify Submit API .. threadpool: simplify Submit API In an effort to modernize our codebase, I'm working to reduce the usage of boost::bind and std::bind. They can be actively harmful[1], and by comparison, lambdas offer the compiler more opportunities to inline. Also, std::bind tends to be aggressive with using allocations for storage, though boost::bind is better about this. This patch modifies ThreadPool to expose just a single Submit API which expects an std::function. Furthermore, all callers have been converted away from bind-based function creation and towards lambdas, which implicitly cast to std::function. There are contortions in a couple places to accommodate the lack of C++14 lambda support (e.g. we need capture-by-move semantics in RaftConsensus), but these are minimal. 1. https://abseil.io/tips/108 Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d --- M src/kudu/codegen/compilation_manager.cc M src/kudu/consensus/consensus-test-util.h M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/fs/dir_manager.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/rpc/reactor.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/thrift/client.h M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_checksum.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/table_scanner.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/util/countdown_latch-test.cc M src/kudu/util/curl_util-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/net/dns_resolver.cc M src/kudu/util/net/dns_resolver.h M src/kudu/util/threadpool-test.cc M src/kudu/util/threadpool.cc M src/kudu/util/threadpool.h 27 files changed, 264 insertions(+), 361 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15401/4 -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] threadpool: simplify Submit API
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15401 to look at the new patch set (#3). Change subject: threadpool: simplify Submit API .. threadpool: simplify Submit API In an effort to modernize our codebase, I'm working to reduce the usage of boost::bind and std::bind. They can be actively harmful[1], and by comparison, lambdas offer the compiler more opportunities to inline. Also, std::bind tends to be aggressive with using allocations for storage, though boost::bind is better about this. This patch modifies ThreadPool to expose just a single Submit API which expects an std::function. Furthermore, all callers have been converted away from bind-based function creation and towards lambdas, which implicitly cast to std::function. There are contortions in a couple places to accommodate the lack of C++14 lambda support (e.g. we need capture-by-move semantics in RaftConsensus), but these are minimal. 1. https://abseil.io/tips/108 Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d --- M src/kudu/codegen/compilation_manager.cc M src/kudu/consensus/consensus-test-util.h M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/fs/dir_manager.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/rpc/reactor.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/thrift/client.h M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_checksum.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/table_scanner.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/util/countdown_latch-test.cc M src/kudu/util/curl_util-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/net/dns_resolver.cc M src/kudu/util/net/dns_resolver.h M src/kudu/util/threadpool-test.cc M src/kudu/util/threadpool.cc M src/kudu/util/threadpool.h 27 files changed, 256 insertions(+), 360 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15401/3 -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] threadpool: simplify Submit API
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15401 ) Change subject: threadpool: simplify Submit API .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/15401/2/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/15401/2/src/kudu/tserver/ts_tablet_manager.cc@890 PS2, Line 890: std::function > Would rather not unless I did it across the board (i.e. plenty of other fil SGTM, let's keep it as is then. -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 11 Mar 2020 22:59:09 + Gerrit-HasComments: Yes
[kudu-CR] threadpool: simplify Submit API
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15401 ) Change subject: threadpool: simplify Submit API .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/15401/2/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/15401/2/src/kudu/consensus/consensus_queue.cc@1512 PS2, Line 1512: = > Does it make a difference whether this is 'this' or '='? In this case I don't think it'll have any effect (since 'this' is the only thing that _can_ be captured). And I used '=' so as to be consistent with the other equivalent methods. http://gerrit.cloudera.org:8080/#/c/15401/2/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/15401/2/src/kudu/tserver/ts_tablet_manager.cc@890 PS2, Line 890: std::function > nit: maybe add 'using std::function' and drop the namespace prefix here and Would rather not unless I did it across the board (i.e. plenty of other files refer to it as std::function). -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 11 Mar 2020 22:46:07 + Gerrit-HasComments: Yes
[kudu-CR] threadpool: simplify Submit API
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15401 ) Change subject: threadpool: simplify Submit API .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/15401/2/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/15401/2/src/kudu/tserver/ts_tablet_manager.cc@890 PS2, Line 890: std::function nit: maybe add 'using std::function' and drop the namespace prefix here and elsewhere in this file? -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 11 Mar 2020 21:33:03 + Gerrit-HasComments: Yes
[kudu-CR] threadpool: simplify Submit API
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15401 ) Change subject: threadpool: simplify Submit API .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/15401/2/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/15401/2/src/kudu/consensus/consensus_queue.cc@1512 PS2, Line 1512: = Does it make a difference whether this is 'this' or '='? -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 11 Mar 2020 20:34:30 + Gerrit-HasComments: Yes
[kudu-CR] threadpool: simplify Submit API
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15401 to look at the new patch set (#2). Change subject: threadpool: simplify Submit API .. threadpool: simplify Submit API In an effort to modernize our codebase, I'm working to reduce the usage of boost::bind and std::bind. They can be actively harmful[1], and by comparison, lambdas offer the compiler more opportunities to inline. Also, std::bind tends to be aggressive with using allocations for storage, though boost::bind is better about this. This patch modifies ThreadPool to expose just a single Submit API which expects an std::function. Furthermore, all callers have been converted away from bind-based function creation and towards lambdas, which implicitly cast to std::function. There are contortions in a couple places to accommodate the lack of C++14 lambda support (e.g. we need capture-by-move semantics in RaftConsensus), but these are minimal. 1. https://abseil.io/tips/108 Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d --- M src/kudu/codegen/compilation_manager.cc M src/kudu/consensus/consensus-test-util.h M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/fs/dir_manager.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/rpc/reactor.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/thrift/client.h M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_checksum.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/table_scanner.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/util/countdown_latch-test.cc M src/kudu/util/curl_util-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/net/dns_resolver.cc M src/kudu/util/net/dns_resolver.h M src/kudu/util/threadpool-test.cc M src/kudu/util/threadpool.cc M src/kudu/util/threadpool.h 27 files changed, 256 insertions(+), 360 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15401/2 -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] threadpool: simplify Submit API
Hello Alexey Serbin, Andrew Wong, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/15401 to review the following change. Change subject: threadpool: simplify Submit API .. threadpool: simplify Submit API In an effort to modernize our codebase, I'm working to reduce the usage of boost::bind and std::bind. They can be actively harmful[1], and by comparison, lambdas offer the compiler more opportunities to inline. Also, std::bind tends to be aggressive with using allocations for storage, though boost::bind is better about this. This patch modifies ThreadPool to expose just a single Submit API which expects an std::function. Furthermore, all callers have been converted away from bind-based function creation and towards lambdas, which implicitly cast to std::function. There are contortions in a couple places to accommodate the lack of C++14 lambda support (e.g. we need capture-by-move semantics in RaftConsensus), but these are minimal. Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d --- M src/kudu/codegen/compilation_manager.cc M src/kudu/consensus/consensus-test-util.h M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/fs/dir_manager.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/rpc/reactor.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/thrift/client.h M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_checksum.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/table_scanner.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/util/countdown_latch-test.cc M src/kudu/util/curl_util-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/net/dns_resolver.cc M src/kudu/util/net/dns_resolver.h M src/kudu/util/threadpool-test.cc M src/kudu/util/threadpool.cc M src/kudu/util/threadpool.h 27 files changed, 256 insertions(+), 360 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15401/1 -- To view, visit http://gerrit.cloudera.org:8080/15401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I2bd2d59809225e4cde7e273e95428478a282aa2d Gerrit-Change-Number: 15401 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Todd Lipcon
