[kudu-CR] WIP: threadpool: token-based sequencing
Adar Dembo has posted comments on this change. Change subject: WIP: threadpool: token-based sequencing .. Patch Set 1: > (1 comment) > - does this affect the performance of a normal ThreadPool that > doesn't use the token feature? From looking at the code it seems > like it would just be a few branches and no extra allocation, etc, > but would be good to be sure of that Yes, that was my intent. However, we can also go in one of two other directions: 1. Recognize that there's no real use case for token and non-token submission in the same pool and further split them up. Some ideas here: a. A common ThreadPool interface with two implementations, sharing code where possible. b. Expose the difference in behavior via templatization (i.e. traits). Neither of these approaches are particularly clean due to the difference in Submit() and Wait() (though I'll admit I didn't think about it very hard) which is why I haven't pursued them yet. 2. Reduce code and complexity by implementing non-token submission as token-based submission. Dan argued for this when he and were discussing the idea yesterday; it's not a bad one if we further optimize token-based submission and avoid allocations. > - not sure about using a string for a token vs having a more opaque > token, such as ThreadPool::ObtainSequencer() returning a > 'Sequencer' struct (which could well be just an int or somesuch) Yes, Dan suggested something similar (though he went a step further and suggested that Submit()/Wait() should happen through this opaque "handle"). The advantage of using an int is that we could convert the map of token queues into a array or vector, and explicit token lifecycle means we can probably avoid shared ownership on token queues. The disadvantage is that now ThreadPool users have to manage the lifecycle of the token, though you could argue they had to already in that they probably must call WaitFor(token) before destructing anyway. > - docs wise, would be good to document fairness and > starvation-freedom properties. Particularly, the fact that this > exhibits neither :-D Probably fine since I think we would typically > use this feature in threadpools with no max thread count specified. > (is it worth actually prohibiting use of tokens in a pool with a > max thread count? or is it just "YMMV" situation?) Agreed. The current implementation prioritizes non-token over token submission. Submission order is preserved for all tasks belonging to a token. The use of a FIFO for token rotation means one busy token can't starve the rest. Note that "no max thread count" means "max of threads", so it's very likely that num_tokens > num_cpus. -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] WIP: threadpool: token-based sequencing
Todd Lipcon has posted comments on this change. Change subject: WIP: threadpool: token-based sequencing .. Patch Set 1: (1 comment) this seems like a reasonable approach. Potential concerns for discussion: - does this affect the performance of a normal ThreadPool that doesn't use the token feature? From looking at the code it seems like it would just be a few branches and no extra allocation, etc, but would be good to be sure of that - not sure about using a string for a token vs having a more opaque token, such as ThreadPool::ObtainSequencer() returning a 'Sequencer' struct (which could well be just an int or somesuch) - docs wise, would be good to document fairness and starvation-freedom properties. Particularly, the fact that this exhibits neither :-D Probably fine since I think we would typically use this feature in threadpools with no max thread count specified. (is it worth actually prohibiting use of tokens in a pool with a max thread count? or is it just "YMMV" situation?) http://gerrit.cloudera.org:8080/#/c/6874/1/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: Line 392: result += c; I think you'd probably want to add a random sleep at the beginning of this lambda, so that if they did accidentally execute in parallel, it would be more likely to actually misorder -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] WIP: threadpool: token-based sequencing
Hello Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/6874 to review the following change. Change subject: WIP: threadpool: token-based sequencing .. WIP: threadpool: token-based sequencing Here's a really rough implementation of token-based sequencing for util/threadpool. The idea is to allow multiple contexts to share a single threadpool with a high number of threads while also ensuring that the pool executes tasks belonging to each context in order. Previously this was only achievable via one-singleton-threadpool-per-context, which grossly inflates the total number of threads. WIP because it's incomplete, is poorly documented, needs a lot more tests, is suboptimal from an allocation perspective (too much map access?) and is probably outright broken. Change-Id: If46dc34212027b6ea5dbc2ead7c7af8be57f2c70 --- M src/kudu/util/threadpool-test.cc M src/kudu/util/threadpool.cc M src/kudu/util/threadpool.h 3 files changed, 158 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/6874/1 -- To view, visit http://gerrit.cloudera.org:8080/6874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If46dc34212027b6ea5dbc2ead7c7af8be57f2c70 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Todd Lipcon
