Adar Dembo has posted comments on this change. Change subject: threadpool: token-based task sequencing ......................................................................
Patch Set 8: -Code-Review (14 comments) http://gerrit.cloudera.org:8080/#/c/6874/8/src/kudu/util/debug-util.h File src/kudu/util/debug-util.h: PS8, Line 105: bool HasCollected() const { > docs Done http://gerrit.cloudera.org:8080/#/c/6874/8/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: Line 40: #include "kudu/util/threadpool.h" > warning: #includes are not sorted properly [llvm-include-order] Done PS8, Line 418: t3 = std::move(t1); > agree with tidy bot that these uses after move are weird But how else am I to test that the move constructor and move assignment operator properly "zero out" the moved instance? To squelch the clang-tidy warnings I added NOLINT to these lines. http://gerrit.cloudera.org:8080/#/c/6874/8/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: Line 130: const SequenceToken::SlotIndex SequenceToken::kInvalidSlotIndex; > warning: redundant 'kInvalidSlotIndex' declaration [readability-redundant-d Not redundant; if I remove this, threadpool-test fails to link. PS8, Line 173: (std::shared_ptr<Runnable> > nit: make_shared? Done PS8, Line 188: pool_->CheckNotPoolThreadUnlocked(); > what is the point of this check? can you leave a comment? It's called in every user-facing function that may block, to ensure that the caller isn't a ThreadPool worker thread. I don't think it makes sense for there to be a comment explaining that in every call-site; best to look at the (short) implementation to learn what's being checked. PS8, Line 258: s->state() == ThreadPool::SequenceSlot::State::RUNNABLE || : s->state() == ThreadPool::SequenceSlot::State::RUNNING || : s->state() == ThreadPool::SequenceSlot::State::QUIESCING > isn't this equivalent to IsActive() Yeah, will replace. PS8, Line 275: s->state() == ThreadPool::SequenceSlot::State::RUNNABLE || : s->state() == ThreadPool::SequenceSlot::State::RUNNING || : s->state() == ThreadPool::SequenceSlot::State::QUIESCING > same. Done http://gerrit.cloudera.org:8080/#/c/6874/8/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: PS8, Line 140: Wait variants > shutdown is also thread safe, right? Yes. I went back and forth on whether to call it out; I reasoned folks would interpret "lifecycle functions" as "ctor/dtor and assignment operators". But apparently not, so I'll make it more explicit. PS8, Line 156: Waits until all the tasks submitted via this token are completed and also : // marks the token as unusable for future submissions. > mind reverting this sentence? I reads like we wait first and then close the You mean reversing? Done. PS8, Line 212: Tasks submitted via one token are fairly prioritized with respect to tasks : // submitted via another token. > not sure I understand this... how are tasks on a token prioritized over ano Tokens are processed in a round-robin style. When a worker thread processes a token, it runs just one of its queued tasks. In this way, we can ensure that one token cannot starve other tokens. I'll add more detail. PS8, Line 360: QUIESCED -> AVAILABLE: > why do we need to transition the state back to available? Only AVAILABLE tokens may be returned to clients via AllocateTokenSlot(). If a token remained in QUIESCED forever, it couldn't be reused by another client. PS8, Line 424: StackTrace running_submit_stack; > what's the perf impact of this? I realize this is a debug only thing but we I ran "perf report" on the new fuzz unit test I added and Collect() consumed 1.42% CPU, which I think is reasonable. Line 516: std::ostream& operator<<(std::ostream& o, ThreadPool::SequenceSlot::State s); > warning: redundant 'operator<<' declaration [readability-redundant-declarat This isn't redundant; if I remove it I get compilation errors in threadpool.cc. -- To view, visit http://gerrit.cloudera.org:8080/6874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If46dc34212027b6ea5dbc2ead7c7af8be57f2c70 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
