Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8257 )
Change subject: thread: improve performance of starting threads ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/client/client-test.cc@4805 PS2, Line 4805: SleepFor(MonoDelta::FromMilliseconds(100)); Seems like the ideal value here would be different for TSAN/ASAN than for regular builds. Regardless, I don't really understand why this test needs to start an ever increasing number of threads. Why can't it start some fixed number of threads that loop on CheckRowcount, then ASSERT_EVENTUALLY() on METRIC_rpcs_queue_overflow? http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread-test.cc File src/kudu/util/thread-test.cc: http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread-test.cc@123 PS2, Line 123: std::vector<scoped_refptr<Thread>> threads(1000); ~Thread doesn't Join(), so shouldn't we explicitly join at the end of the test? Otherwise the detached threads will linger into the next test, no? http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc File src/kudu/util/thread.cc: http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@a589 PS2, Line 589: Curious why you moved this out of here and into StartThread? http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@456 PS2, Line 456: if (Thread::current_thread() && : Thread::current_thread()->tid() == thread_->tid()) { : return Status::InvalidArgument("Can't join on own thread", thread_->name_); : } Given tid()'s new behavior, should we demote this to a DCHECK? http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@517 PS2, Line 517: std::string Thread::ToString() const { I presume we don't have ToString() in any hot path? http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@553 PS2, Line 553: holder->swap(old_holder_val); Surely there's no use case for this? 'holder' is supposed to be a pure OUT parameter. http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@575 PS2, Line 575: // If we failed to create the thread, we need to undo all of our prep work. Could you use a MakeScopedCleanup for this, and cancel it on success? It's not fewer LOC but it's a very recognizable pattern. http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@591 PS2, Line 591: t->tid() This doesn't seem like something we'd want here, given tid()'s new behavior. http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@617 PS2, Line 617: thread_manager->SetThreadName(name, t->tid()); : thread_manager->AddThread(pthread_self(), name, t->category(), t->tid()); Could use tid_ here instead. http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@646 PS2, Line 646: VLOG(2) << "Ended thread " << t->tid() << " - " Could use tid_ here instead too. http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@648 PS2, Line 648: t->Release(); Maybe add a brief comment explaining that this could cause t to destruct, so it's important nothing accesses it afterwards. -- To view, visit http://gerrit.cloudera.org:8080/8257 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879 Gerrit-Change-Number: 8257 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 11 Oct 2017 17:54:03 +0000 Gerrit-HasComments: Yes
