Adar Dembo has posted comments on this change. Change subject: threadpool: token-based task sequencing ......................................................................
Patch Set 2: (15 comments) http://gerrit.cloudera.org:8080/#/c/6874/4//COMMIT_MSG Commit Message: Line 7: threadpool: token-based task sequencing > nit: consider keeping length of lines under 72 chars: I wrap at 76, so that, after the 4 char indentation automatically added by git, the lines are under 80 and thus fit into a standard 80x24 terminal window. PS4, Line 10: n > ditto Done PS4, Line 10: m > nit: would it be better for readability if this letter were capital? Done http://gerrit.cloudera.org:8080/#/c/6874/4/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: PS4, Line 563: : TEST_F(ThreadPoolTest, TestTokenSubmissionsAdhereToMaxQueueSize) { : A > What happens if an assertion fires above? Is it necessary to wait for toke You're right; I should wait on them. I'll move this into a scoped cleanup. I adjusted the cleanup for some other tests too. http://gerrit.cloudera.org:8080/#/c/6874/4/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: Line 139: SequenceToken::SequenceToken(SequenceToken&& t) > warning: move constructors should be marked noexcept [misc-noexcept-move-co Done Line 145: SequenceToken& SequenceToken::operator=(SequenceToken&& t) { > warning: move assignment operators should be marked noexcept [misc-noexcept Done PS4, Line 328: SequenceToken::SlotIndex idx = -1; > Is there a restriction on maximum number of allocated token slots? No. Nor do I see the need to add one. PS4, Line 345: : return SequenceToken(this, idx); : } : : void ThreadPool::ReleaseTokenSlot(SequenceToken* t) { : { : // Release the slot with the lock held. > Regarding this transition from DEALLOCATED to INACTIVE for non-existing slo So why do we actually need both DEALLOCATED and INACTIVE? Because we need to distinguish between "slot exists in slots_ but has no token" and "slot exists in slots_ and there exists a token for it". If we don't distinguish between the two, we won't be able to reuse slots for future tokens. It's true that new_slot could start in INACTIVE instead of DEALLOCATED, but I wanted the state transitions for "slot just allocated on the heap" and "slot reused" to be identical, which is why they both go from DEALLOCATED to INACTIVE. As for avoiding the condvar broadcast in the DEALLOCATED->INACTIVE transition: we could avoid it, but it should be a no-op anyway, because the condvar is specific to this slot, and since there's no token for it, there should be no waiters either. So I didn't think it mattered and opted to keep the state transition code simple. BTW, I'm going to rename the states to ones that are intuitive. http://gerrit.cloudera.org:8080/#/c/6874/4/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: PS4, Line 21: #include <list> > nit: not sure this is needed Done PS4, Line 24: #include <unordered_map> > nit: not sure this is needed Done Line 144: SequenceToken(SequenceToken&& t); > warning: move constructors should be marked noexcept [misc-noexcept-move-co Done Line 145: SequenceToken& operator=(SequenceToken&& t); > warning: move assignment operators should be marked noexcept [misc-noexcept Done PS4, Line 157: binded > nit: bound? Copied from ThreadPool::SubmitFunc(), I'll correct both. PS4, Line 182: int > nit: why not to use the newly introduced typedef here as well (and re-order Done PS4, Line 182: kInvalidSlotIndex > nit: since it's an integral type and we are using C++11, consider defining Done -- 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: 2 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: 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
