Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1423773003:
Add SequencedTaskRunnerHandle to get a SequencedTaskRunner for the current thread / sequence. (Closed)
|
Created:
5 years, 2 months ago by Bernhard Bauer Modified:
5 years, 1 month ago CC:
chromium-reviews, vmpstr+watch_chromium.org, robliao Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd SequencedTaskRunnerHandle to get a SequencedTaskRunner for the current thread / sequence.
BUG=546596
Committed: https://crrev.com/8a4087226c482fadb886585d97081adf379e6b68
Cr-Commit-Position: refs/heads/master@{#357067}
Patch Set 1 #Patch Set 2 : . #
Total comments: 2
Patch Set 3 : review #
Total comments: 3
Patch Set 4 : add unittest #Patch Set 5 : add comment #
Total comments: 2
Patch Set 6 : comment #
Total comments: 22
Patch Set 7 : review #Patch Set 8 : review #Patch Set 9 : export SequenceToken #
Total comments: 2
Messages
Total messages: 47 (9 generated)
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
https://codereview.chromium.org/1423773003/diff/20001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.h (right): https://codereview.chromium.org/1423773003/diff/20001/base/threading/sequence... base/threading/sequenced_task_runner_handle.h:25: static scoped_refptr<SequencedTaskRunner> Get(); Could this follow the same IsSet/Get contract of ThreadTaskRunnerHandle and crash if Get is called without an active task runner? I think it's better to crash early than later when the null pointer is used.
gab@chromium.org changed reviewers: + gab@chromium.org
drive-by lgtm -- I think most users of ThreadTaskRunnerHandle should be using this instead (I doubt very many, if any, have thread-affinity requirement -- most things in Chrome only have sequence-affinity, or actually not even sequence, but thread-safe (i.e. posting order is often not even important it's just lack of parallelism that is + memory barriers between tasks)). https://codereview.chromium.org/1423773003/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1423773003/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:35: base::ThreadTaskRunnerHandle::IsSet(); nit: missing a space.
https://codereview.chromium.org/1423773003/diff/20001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.h (right): https://codereview.chromium.org/1423773003/diff/20001/base/threading/sequence... base/threading/sequenced_task_runner_handle.h:25: static scoped_refptr<SequencedTaskRunner> Get(); On 2015/10/23 15:47:33, Sami wrote: > Could this follow the same IsSet/Get contract of ThreadTaskRunnerHandle and > crash if Get is called without an active task runner? I think it's better to > crash early than later when the null pointer is used. Done.
On 2015/10/23 16:19:21, gab wrote: > drive-by lgtm -- I think most users of ThreadTaskRunnerHandle should be using > this instead (I doubt very many, if any, have thread-affinity requirement -- > most things in Chrome only have sequence-affinity, or actually not even > sequence, but thread-safe (i.e. posting order is often not even important it's > just lack of parallelism that is + memory barriers between tasks)). Out of curiosity, what's the difference between sequencing behavior and that flavor of thread safety? Just running tasks out of order? https://codereview.chromium.org/1423773003/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1423773003/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:35: base::ThreadTaskRunnerHandle::IsSet(); On 2015/10/23 16:19:21, gab wrote: > nit: missing a space. Clang-format did that (I think it's aligning the right side of the or-operator with the left side, which is a parenthesized expression. Happy to change it though if you want me to.
On 2015/10/23 16:24:00, Bernhard Bauer wrote: > On 2015/10/23 16:19:21, gab wrote: > > drive-by lgtm -- I think most users of ThreadTaskRunnerHandle should be using > > this instead (I doubt very many, if any, have thread-affinity requirement -- > > most things in Chrome only have sequence-affinity, or actually not even > > sequence, but thread-safe (i.e. posting order is often not even important it's > > just lack of parallelism that is + memory barriers between tasks)). > > Out of curiosity, what's the difference between sequencing behavior and that > flavor of thread safety? Just running tasks out of order? In a world of queues with no concept of priorities there is no difference in the execution logic between executing tasks in a thread-safe way (not in parallel but in any order) versus in a sequenced-way. But if you take a look at the proposed Executor API I linked to on the bug, it begins to matter (see SERIAL mode). All I'm saying here is that I think most things in Chrome do not have thread-affinity nor sequence-affinity, but rather only thread-safe requirements (i.e. one tasks at a time with a memory barrier before running the next one on another thread -- the barrier being implicit in the lock used to access the task queue anyways). https://codereview.chromium.org/1423773003/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1423773003/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:35: base::ThreadTaskRunnerHandle::IsSet(); On 2015/10/23 16:24:00, Bernhard Bauer wrote: > On 2015/10/23 16:19:21, gab wrote: > > nit: missing a space. > > Clang-format did that (I think it's aligning the right side of the or-operator > with the left side, which is a parenthesized expression. Happy to change it > though if you want me to. As sorry missed the extra bracket -- clang > gab :-)
On 2015/10/23 16:39:10, gab wrote: > On 2015/10/23 16:24:00, Bernhard Bauer wrote: > > On 2015/10/23 16:19:21, gab wrote: > > > drive-by lgtm -- I think most users of ThreadTaskRunnerHandle should be > using > > > this instead (I doubt very many, if any, have thread-affinity requirement -- > > > most things in Chrome only have sequence-affinity, or actually not even > > > sequence, but thread-safe (i.e. posting order is often not even important > it's > > > just lack of parallelism that is + memory barriers between tasks)). > > > > Out of curiosity, what's the difference between sequencing behavior and that > > flavor of thread safety? Just running tasks out of order? > > In a world of queues with no concept of priorities there is no difference in the > execution logic between executing tasks in a thread-safe way (not in parallel > but in any order) versus in a sequenced-way. > > But if you take a look at the proposed Executor API I linked to on the bug, it > begins to matter (see SERIAL mode). Gotcha. Yeah, I wasn't quite sure why someone would *want* out-of-order execution, unless the API allows priorities 😃 > All I'm saying here is that I think most things in Chrome do not have > thread-affinity nor sequence-affinity, but rather only thread-safe requirements > (i.e. one tasks at a time with a memory barrier before running the next one on > another thread -- the barrier being implicit in the lock used to access the task > queue anyways). Yes, agreed.
bauerb@chromium.org changed reviewers: + danakj@chromium.org
OK, let's do this 😃 Dana, please review.
Description was changed from ========== Add SequencedTaskRunnerHandle to get a SequencedTaskRunner for the current thread / sequence. BUG=546596 ========== to ========== Add SequencedTaskRunnerHandle to get a SequencedTaskRunner for the current thread / sequence. BUG=546596 ==========
On 2015/10/26 14:08:31, Bernhard Bauer wrote: > OK, let's do this 😃 Dana, please review. [CC robliao] Shall we file a bug to also move existing callers to this as much as possible? I'm happy to help make this happen :-) -- the outcome of how many callers actually care about thread-affinity will help guide the aforementioned Executor proposal. Thanks, Gab
On 2015/10/26 19:23:28, gab wrote: > On 2015/10/26 14:08:31, Bernhard Bauer wrote: > > OK, let's do this 😃 Dana, please review. > > [CC robliao] > > Shall we file a bug to also move existing callers to this as much as possible? > I'm happy to help make this happen :-) -- the outcome of how many callers > actually care about thread-affinity will help guide the aforementioned Executor > proposal. > > Thanks, > Gab In principle yes, but there are about 2500 call sites to ThreadTaskRunnerHandle::Get(); I don't think we'll be able to manually audit all of them...
On 2015/10/27 15:14:38, Bernhard Bauer wrote: > On 2015/10/26 19:23:28, gab wrote: > > On 2015/10/26 14:08:31, Bernhard Bauer wrote: > > > OK, let's do this 😃 Dana, please review. > > > > [CC robliao] > > > > Shall we file a bug to also move existing callers to this as much as possible? > > I'm happy to help make this happen :-) -- the outcome of how many callers > > actually care about thread-affinity will help guide the aforementioned > Executor > > proposal. > > > > Thanks, > > Gab > > In principle yes, but there are about 2500 call sites to > ThreadTaskRunnerHandle::Get(); I don't think we'll be able to manually audit all > of them... Oh that's more than I expected.. then I would at least like this CL to add a comment on the ThreadTaskRunnerHandle API mentioning that it should only be used if thread-affinity is required and to prefer SequencedTaskRunnerHandle otherwise.
On 2015/10/27 15:21:59, gab wrote: > On 2015/10/27 15:14:38, Bernhard Bauer wrote: > > On 2015/10/26 19:23:28, gab wrote: > > > On 2015/10/26 14:08:31, Bernhard Bauer wrote: > > > > OK, let's do this 😃 Dana, please review. > > > > > > [CC robliao] > > > > > > Shall we file a bug to also move existing callers to this as much as > possible? > > > I'm happy to help make this happen :-) -- the outcome of how many callers > > > actually care about thread-affinity will help guide the aforementioned > > Executor > > > proposal. > > > > > > Thanks, > > > Gab > > > > In principle yes, but there are about 2500 call sites to > > ThreadTaskRunnerHandle::Get(); I don't think we'll be able to manually audit > all > > of them... > > Oh that's more than I expected.. then I would at least like this CL to add a > comment on the ThreadTaskRunnerHandle API mentioning that it should only be used > if thread-affinity is required and to prefer SequencedTaskRunnerHandle > otherwise. Done.
https://codereview.chromium.org/1423773003/diff/80001/base/thread_task_runner... File base/thread_task_runner_handle.h (right): https://codereview.chromium.org/1423773003/diff/80001/base/thread_task_runner... base/thread_task_runner_handle.h:22: // posted tasks run after each other), use SequencedTaskRunnerHandle instead. I suggest changing all of this new text with: // Prefer SequenceTaskRunnerHandle to this unless thread affinity is required. (i.e. I prefer a quick and strong suggestion -- as for talking about SequencedWorkerPool I think it's implicit and worst case developers will find out the same way you have..?)
Dana, ping? https://codereview.chromium.org/1423773003/diff/80001/base/thread_task_runner... File base/thread_task_runner_handle.h (right): https://codereview.chromium.org/1423773003/diff/80001/base/thread_task_runner... base/thread_task_runner_handle.h:22: // posted tasks run after each other), use SequencedTaskRunnerHandle instead. On 2015/10/27 16:57:58, gab wrote: > I suggest changing all of this new text with: > > // Prefer SequenceTaskRunnerHandle to this unless thread affinity is required. > > (i.e. I prefer a quick and strong suggestion -- as for talking about > SequencedWorkerPool I think it's implicit and worst case developers will find > out the same way you have..?) Done.
https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle_unittest.cc (right): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_task_runner_handle_unittest.cc:62: namespace { nit: you could wrap the whole file in anon to make things easier namespace base { namespace { ..everything.. } } https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_task_runner_handle_unittest.cc:83: } // namespace base 2 spaces before // https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:266: const scoped_refptr<SequencedWorkerPool>& worker_pool() const { just return type scoped_refptr<SWP>? https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:272: g_lazy_tls_ptr; "Data members of classes, both static and non-static, are named like ordinary nonmember variables, but with a trailing underscore." Remove the g_ and add a trailing _. https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.h (left): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:164: // Pass the maximum number of threads (they will be lazily created as needed) you can let comments wrap at 80 https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:133: bool operator==(const SequenceToken& other) const { Why do we need this? https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:166: static SequenceToken GetSequenceTokenForCurrentThread(); I think this changes the behaviour of this method subtley, in that after a task ran, it would continue to return the same SequenceToken before? The comment makes no claim at this being important, but did you consider that? Or did I misread the existing code? https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:372: BASE_EXPORT std::ostream& operator<<( Do we need this? https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:918: EXPECT_EQ(expected_pool, pool); you could compare the .get()s and avoid the need for the ostream operator?
https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle_unittest.cc (right): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_task_runner_handle_unittest.cc:62: namespace { On 2015/10/27 20:03:20, danakj wrote: > nit: you could wrap the whole file in anon to make things easier > > namespace base { > namespace { > > ..everything.. > > } > } Done. https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_task_runner_handle_unittest.cc:83: } // namespace base On 2015/10/27 20:03:20, danakj wrote: > 2 spaces before // Done. https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:266: const scoped_refptr<SequencedWorkerPool>& worker_pool() const { On 2015/10/27 20:03:20, danakj wrote: > just return type scoped_refptr<SWP>? Done. https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:272: g_lazy_tls_ptr; On 2015/10/27 20:03:20, danakj wrote: > "Data members of classes, both static and non-static, are named like ordinary > nonmember variables, but with a trailing underscore." > > Remove the g_ and add a trailing _. Done. https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.h (left): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:164: // Pass the maximum number of threads (they will be lazily created as needed) On 2015/10/27 20:03:20, danakj wrote: > you can let comments wrap at 80 Done. https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:133: bool operator==(const SequenceToken& other) const { On 2015/10/27 20:03:20, danakj wrote: > Why do we need this? {DCHECK,EXPECT,ASSERT}_EQ use the equality operator. I think having those (rather than DCHECK(... == ...) is useful enough to warrant the addition. https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:166: static SequenceToken GetSequenceTokenForCurrentThread(); On 2015/10/27 20:03:20, danakj wrote: > I think this changes the behaviour of this method subtley, in that after a task > ran, it would continue to return the same SequenceToken before? The comment > makes no claim at this being important, but did you consider that? Or did I > misread the existing code? Uh, possibly 😃 Basically, the value that is stored in thread-local storage is a _pointer_ (in the existing code) to the |task_sequence_token_| member of the current worker, so it can be updated when processing a new task. And accessing it while no task is running is not allowed (there is a DCHECK in SequencedWorkerPool::Worker::task_sequence_token()). And none of that changes with my change, I just change where the TLS pointer points to. https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:372: BASE_EXPORT std::ostream& operator<<( On 2015/10/27 20:03:20, danakj wrote: > Do we need this? Same as above, this is handy for tests and DCHECKs. https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:918: EXPECT_EQ(expected_pool, pool); On 2015/10/27 20:03:20, danakj wrote: > you could compare the .get()s and avoid the need for the ostream operator? No, that's the ostream operator on scoped_refptr. The ostream operator I'm defining is on the sequence tokens.
https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:133: bool operator==(const SequenceToken& other) const { On 2015/10/28 13:36:28, Bernhard Bauer wrote: > On 2015/10/27 20:03:20, danakj wrote: > > Why do we need this? > > {DCHECK,EXPECT,ASSERT}_EQ use the equality operator. I think having those > (rather than DCHECK(... == ...) is useful enough to warrant the addition. I might agree if the operator was only compiled into tests. I don't think we should be adding operators otherwise generally. The style guide also says not to. You can do DCHECK(a == b) << a.ToString() << " " << b.ToString(); or something if you need to. https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:166: static SequenceToken GetSequenceTokenForCurrentThread(); On 2015/10/28 13:36:28, Bernhard Bauer wrote: > On 2015/10/27 20:03:20, danakj wrote: > > I think this changes the behaviour of this method subtley, in that after a > task > > ran, it would continue to return the same SequenceToken before? The comment > > makes no claim at this being important, but did you consider that? Or did I > > misread the existing code? > > Uh, possibly 😃 Basically, the value that is stored in thread-local storage is a > _pointer_ (in the existing code) to the |task_sequence_token_| member of the > current worker, so it can be updated when processing a new task. And accessing > it while no task is running is not allowed (there is a DCHECK in > SequencedWorkerPool::Worker::task_sequence_token()). And none of that changes > with my change, I just change where the TLS pointer points to. Ah, thanks :) https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:372: BASE_EXPORT std::ostream& operator<<( On 2015/10/28 13:36:28, Bernhard Bauer wrote: > On 2015/10/27 20:03:20, danakj wrote: > > Do we need this? > > Same as above, this is handy for tests and DCHECKs. Similar comment. Can you define this only in the test target?
https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/1423773003/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:133: bool operator==(const SequenceToken& other) const { On 2015/10/28 18:08:15, danakj wrote: > On 2015/10/28 13:36:28, Bernhard Bauer wrote: > > On 2015/10/27 20:03:20, danakj wrote: > > > Why do we need this? > > > > {DCHECK,EXPECT,ASSERT}_EQ use the equality operator. I think having those > > (rather than DCHECK(... == ...) is useful enough to warrant the addition. > > I might agree if the operator was only compiled into tests. I don't think we > should be adding operators otherwise generally. The style guide also says not > to. You can do DCHECK(a == b) << a.ToString() << " " << b.ToString(); or > something if you need to. Ok. Turns out the easiest thing is to just compare the string values ☺
LGTM
On 2015/10/28 20:49:43, danakj wrote: > LGTM Thanks for the review!
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1423773003/#ps140001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1423773003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1423773003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/1423773003/#ps160001 (title: "export SequenceToken")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1423773003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1423773003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8a4087226c482fadb886585d97081adf379e6b68 Cr-Commit-Position: refs/heads/master@{#357067}
Message was sent while issue was closed.
Just thought of something in the midst of the chromium-dev discussion. https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... base/threading/sequenced_task_runner_handle.cc:22: DCHECK(sequence_token.IsValid()); Actually, if the |sequence_token| is invalid: doesn't that mean that the current task was posted one-off (not on a sequence)? If so, we should still return a valid SequenceTaskRunner (with a new token) here, right?
Message was sent while issue was closed.
https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... base/threading/sequenced_task_runner_handle.cc:22: DCHECK(sequence_token.IsValid()); On 2015/11/02 18:42:40, gab wrote: > Actually, if the |sequence_token| is invalid: doesn't that mean that the current > task was posted one-off (not on a sequence)? If so, we should still return a > valid SequenceTaskRunner (with a new token) here, right? Maybe I'm confused, but I thought the idea was that you'd use this only from within a task running on a SWP, which means it would be valid. Are you saying you can call this from any place and expect some SWP to pick it up? (which one?)
Message was sent while issue was closed.
On 2015/11/02 18:44:53, danakj wrote: > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > File base/threading/sequenced_task_runner_handle.cc (right): > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > base/threading/sequenced_task_runner_handle.cc:22: > DCHECK(sequence_token.IsValid()); > On 2015/11/02 18:42:40, gab wrote: > > Actually, if the |sequence_token| is invalid: doesn't that mean that the > current > > task was posted one-off (not on a sequence)? If so, we should still return a > > valid SequenceTaskRunner (with a new token) here, right? > > Maybe I'm confused, but I thought the idea was that you'd use this only from > within a task running on a SWP, which means it would be valid. Are you saying > you can call this from any place and expect some SWP to pick it up? (which one?) No, I meant the SWP also has a PostTask() entry-point (you can post tasks to the pool without asking being tied to a Sequencetoken).
Message was sent while issue was closed.
On Mon, Nov 2, 2015 at 11:11 AM, <gab@chromium.org> wrote: > On 2015/11/02 18:44:53, danakj wrote: > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > >> File base/threading/sequenced_task_runner_handle.cc (right): >> > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > >> base/threading/sequenced_task_runner_handle.cc:22: >> DCHECK(sequence_token.IsValid()); >> On 2015/11/02 18:42:40, gab wrote: >> > Actually, if the |sequence_token| is invalid: doesn't that mean that the >> current >> > task was posted one-off (not on a sequence)? If so, we should still >> return a >> > valid SequenceTaskRunner (with a new token) here, right? >> > > Maybe I'm confused, but I thought the idea was that you'd use this only >> from >> within a task running on a SWP, which means it would be valid. Are you >> saying >> you can call this from any place and expect some SWP to pick it up? (which >> > one?) > > No, I meant the SWP also has a PostTask() entry-point (you can post tasks > to the > pool without asking being tied to a Sequencetoken). > OIC that does sound right then, and easy to express with a test. Harder question seems to be how to we encode expected shutdown behaviour into this API? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/11/02 21:51:38, danakj wrote: > On Mon, Nov 2, 2015 at 11:11 AM, <mailto:gab@chromium.org> wrote: > > > On 2015/11/02 18:44:53, danakj wrote: > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > >> File base/threading/sequenced_task_runner_handle.cc (right): > >> > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > >> base/threading/sequenced_task_runner_handle.cc:22: > >> DCHECK(sequence_token.IsValid()); > >> On 2015/11/02 18:42:40, gab wrote: > >> > Actually, if the |sequence_token| is invalid: doesn't that mean that the > >> current > >> > task was posted one-off (not on a sequence)? If so, we should still > >> return a > >> > valid SequenceTaskRunner (with a new token) here, right? The problem with this is that we would need to get a sequence token that is guaranteed not to immediately start running on a different worker pool thread, otherwise we'd run the risk of violating sequentiality. > > Maybe I'm confused, but I thought the idea was that you'd use this only > >> from > >> within a task running on a SWP, which means it would be valid. Are you > >> saying > >> you can call this from any place and expect some SWP to pick it up? (which > >> > > one?) > > > > No, I meant the SWP also has a PostTask() entry-point (you can post tasks > > to the > > pool without asking being tied to a Sequencetoken). > > > > OIC that does sound right then, and easy to express with a test. > > Harder question seems to be how to we encode expected shutdown behaviour > into this API? We could add a variant of Get() that takes a ShutdownBehavior and passes it when creating a new SequencedTaskRunner from the worker pool.
Message was sent while issue was closed.
On 2015/11/03 12:47:54, Bernhard Bauer wrote: > On 2015/11/02 21:51:38, danakj wrote: > > On Mon, Nov 2, 2015 at 11:11 AM, <mailto:gab@chromium.org> wrote: > > > > > On 2015/11/02 18:44:53, danakj wrote: > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > >> File base/threading/sequenced_task_runner_handle.cc (right): > > >> > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > >> base/threading/sequenced_task_runner_handle.cc:22: > > >> DCHECK(sequence_token.IsValid()); > > >> On 2015/11/02 18:42:40, gab wrote: > > >> > Actually, if the |sequence_token| is invalid: doesn't that mean that the > > >> current > > >> > task was posted one-off (not on a sequence)? If so, we should still > > >> return a > > >> > valid SequenceTaskRunner (with a new token) here, right? > > The problem with this is that we would need to get a sequence token that is > guaranteed not to immediately start running on a different worker pool thread, > otherwise we'd run the risk of violating sequentiality. You mean sequentiality from the current task? It wasn't posted to a sequence so it doesn't expect sequencing, but I guess you're saying the caller expects his task to be sequenced AFTER the currently running task regardless of whether it is in an explicit sequence? The SequenceWorkerPool could be made to support that it feels. > > > > Maybe I'm confused, but I thought the idea was that you'd use this only > > >> from > > >> within a task running on a SWP, which means it would be valid. Are you > > >> saying > > >> you can call this from any place and expect some SWP to pick it up? (which > > >> > > > one?) > > > > > > No, I meant the SWP also has a PostTask() entry-point (you can post tasks > > > to the > > > pool without asking being tied to a Sequencetoken). > > > > > > > OIC that does sound right then, and easy to express with a test. > > > > Harder question seems to be how to we encode expected shutdown behaviour > > into this API? > > We could add a variant of Get() that takes a ShutdownBehavior and passes it when > creating a new SequencedTaskRunner from the worker pool. I think we need to always inherit the ShutdownBehaviour of the originating sequence, see my reply on chromium-dev (having tasks with different ShutdownBehaviours on the same sequence doesn't make sense). In fact, finding a way to just get back the initial SequencedTaskRunner itself would be even better but I forget whether the SequencedWorkerPool is aware of the originating TaskRunner.
Message was sent while issue was closed.
On 2015/11/03 22:22:33, gab wrote: > On 2015/11/03 12:47:54, Bernhard Bauer wrote: > > On 2015/11/02 21:51:38, danakj wrote: > > > On Mon, Nov 2, 2015 at 11:11 AM, <mailto:gab@chromium.org> wrote: > > > > > > > On 2015/11/02 18:44:53, danakj wrote: > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > >> File base/threading/sequenced_task_runner_handle.cc (right): > > > >> > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > >> base/threading/sequenced_task_runner_handle.cc:22: > > > >> DCHECK(sequence_token.IsValid()); > > > >> On 2015/11/02 18:42:40, gab wrote: > > > >> > Actually, if the |sequence_token| is invalid: doesn't that mean that > the > > > >> current > > > >> > task was posted one-off (not on a sequence)? If so, we should still > > > >> return a > > > >> > valid SequenceTaskRunner (with a new token) here, right? > > > > The problem with this is that we would need to get a sequence token that is > > guaranteed not to immediately start running on a different worker pool thread, > > otherwise we'd run the risk of violating sequentiality. > > You mean sequentiality from the current task? It wasn't posted to a sequence so > it doesn't expect sequencing, but I guess you're saying the caller expects his > task to be sequenced AFTER the currently running task regardless of whether it > is in an explicit sequence? > > The SequenceWorkerPool could be made to support that it feels. Yes, a common use case for STRH (or TTRH before that) is to run something at a later point, for example to run a callback in such a way that the caller doesn't have to deal with reentrancy issues. Calling the callback before the original returns _on a different thread_ would probably break even more 😃 > > > > > > Maybe I'm confused, but I thought the idea was that you'd use this only > > > >> from > > > >> within a task running on a SWP, which means it would be valid. Are you > > > >> saying > > > >> you can call this from any place and expect some SWP to pick it up? > (which > > > >> > > > > one?) > > > > > > > > No, I meant the SWP also has a PostTask() entry-point (you can post tasks > > > > to the > > > > pool without asking being tied to a Sequencetoken). > > > > > > > > > > OIC that does sound right then, and easy to express with a test. > > > > > > Harder question seems to be how to we encode expected shutdown behaviour > > > into this API? > > > > We could add a variant of Get() that takes a ShutdownBehavior and passes it > when > > creating a new SequencedTaskRunner from the worker pool. > > I think we need to always inherit the ShutdownBehaviour of the originating > sequence, see my reply on chromium-dev (having tasks with different > ShutdownBehaviours on the same sequence doesn't make sense). In fact, finding a > way to just get back the initial SequencedTaskRunner itself would be even better > but I forget whether the SequencedWorkerPool is aware of the originating > TaskRunner. It is not; it only stores the sequence token and a pointer to itself on each thread.
Message was sent while issue was closed.
On 2015/11/04 08:36:50, Bernhard Bauer wrote: > On 2015/11/03 22:22:33, gab wrote: > > On 2015/11/03 12:47:54, Bernhard Bauer wrote: > > > On 2015/11/02 21:51:38, danakj wrote: > > > > On Mon, Nov 2, 2015 at 11:11 AM, <mailto:gab@chromium.org> wrote: > > > > > > > > > On 2015/11/02 18:44:53, danakj wrote: > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > > > >> File base/threading/sequenced_task_runner_handle.cc (right): > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > > > >> base/threading/sequenced_task_runner_handle.cc:22: > > > > >> DCHECK(sequence_token.IsValid()); > > > > >> On 2015/11/02 18:42:40, gab wrote: > > > > >> > Actually, if the |sequence_token| is invalid: doesn't that mean that > > the > > > > >> current > > > > >> > task was posted one-off (not on a sequence)? If so, we should still > > > > >> return a > > > > >> > valid SequenceTaskRunner (with a new token) here, right? > > > > > > The problem with this is that we would need to get a sequence token that is > > > guaranteed not to immediately start running on a different worker pool > thread, > > > otherwise we'd run the risk of violating sequentiality. > > > > You mean sequentiality from the current task? It wasn't posted to a sequence > so > > it doesn't expect sequencing, but I guess you're saying the caller expects his > > task to be sequenced AFTER the currently running task regardless of whether it > > is in an explicit sequence? > > > > The SequenceWorkerPool could be made to support that it feels. > > Yes, a common use case for STRH (or TTRH before that) is to run something at a > later point, for example to run a callback in such a way that the caller doesn't > have to deal with reentrancy issues. Calling the callback before the original > returns _on a different thread_ would probably break even more 😃 > > > > > > > > > Maybe I'm confused, but I thought the idea was that you'd use this only > > > > >> from > > > > >> within a task running on a SWP, which means it would be valid. Are you > > > > >> saying > > > > >> you can call this from any place and expect some SWP to pick it up? > > (which > > > > >> > > > > > one?) > > > > > > > > > > No, I meant the SWP also has a PostTask() entry-point (you can post > tasks > > > > > to the > > > > > pool without asking being tied to a Sequencetoken). > > > > > > > > > > > > > OIC that does sound right then, and easy to express with a test. > > > > > > > > Harder question seems to be how to we encode expected shutdown behaviour > > > > into this API? > > > > > > We could add a variant of Get() that takes a ShutdownBehavior and passes it > > when > > > creating a new SequencedTaskRunner from the worker pool. > > > > I think we need to always inherit the ShutdownBehaviour of the originating > > sequence, see my reply on chromium-dev (having tasks with different > > ShutdownBehaviours on the same sequence doesn't make sense). In fact, finding > a > > way to just get back the initial SequencedTaskRunner itself would be even > better > > but I forget whether the SequencedWorkerPool is aware of the originating > > TaskRunner. > > It is not; it only stores the sequence token and a pointer to itself on each > thread. Oh wait, I lied: it does store the shutdown behavior (https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq...).
Message was sent while issue was closed.
On 2015/11/04 08:37:52, Bernhard Bauer wrote: > On 2015/11/04 08:36:50, Bernhard Bauer wrote: > > On 2015/11/03 22:22:33, gab wrote: > > > On 2015/11/03 12:47:54, Bernhard Bauer wrote: > > > > On 2015/11/02 21:51:38, danakj wrote: > > > > > On Mon, Nov 2, 2015 at 11:11 AM, <mailto:gab@chromium.org> wrote: > > > > > > > > > > > On 2015/11/02 18:44:53, danakj wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > > > > > >> File base/threading/sequenced_task_runner_handle.cc (right): > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > > > > > >> base/threading/sequenced_task_runner_handle.cc:22: > > > > > >> DCHECK(sequence_token.IsValid()); > > > > > >> On 2015/11/02 18:42:40, gab wrote: > > > > > >> > Actually, if the |sequence_token| is invalid: doesn't that mean > that > > > the > > > > > >> current > > > > > >> > task was posted one-off (not on a sequence)? If so, we should still > > > > > >> return a > > > > > >> > valid SequenceTaskRunner (with a new token) here, right? > > > > > > > > The problem with this is that we would need to get a sequence token that > is > > > > guaranteed not to immediately start running on a different worker pool > > thread, > > > > otherwise we'd run the risk of violating sequentiality. > > > > > > You mean sequentiality from the current task? It wasn't posted to a sequence > > so > > > it doesn't expect sequencing, but I guess you're saying the caller expects > his > > > task to be sequenced AFTER the currently running task regardless of whether > it > > > is in an explicit sequence? > > > > > > The SequenceWorkerPool could be made to support that it feels. > > > > Yes, a common use case for STRH (or TTRH before that) is to run something at a > > later point, for example to run a callback in such a way that the caller > doesn't > > have to deal with reentrancy issues. Calling the callback before the original > > returns _on a different thread_ would probably break even more 😃 Right, but I'm still pretty sure STRH should be agnostic to whether the current task itself is running as part of a sequence or not. As proposed below I think we want the SequencedTaskRunner to be built by the SequencedWorkerPool itself, in which case it could special case that use-case (i.e. kick off a new sequence which starts with the already ongoing task). > > > > > > > > > > > > Maybe I'm confused, but I thought the idea was that you'd use this > only > > > > > >> from > > > > > >> within a task running on a SWP, which means it would be valid. Are > you > > > > > >> saying > > > > > >> you can call this from any place and expect some SWP to pick it up? > > > (which > > > > > >> > > > > > > one?) > > > > > > > > > > > > No, I meant the SWP also has a PostTask() entry-point (you can post > > tasks > > > > > > to the > > > > > > pool without asking being tied to a Sequencetoken). > > > > > > > > > > > > > > > > OIC that does sound right then, and easy to express with a test. > > > > > > > > > > Harder question seems to be how to we encode expected shutdown behaviour > > > > > into this API? > > > > > > > > We could add a variant of Get() that takes a ShutdownBehavior and passes > it > > > when > > > > creating a new SequencedTaskRunner from the worker pool. > > > > > > I think we need to always inherit the ShutdownBehaviour of the originating > > > sequence, see my reply on chromium-dev (having tasks with different > > > ShutdownBehaviours on the same sequence doesn't make sense). In fact, > finding > > a > > > way to just get back the initial SequencedTaskRunner itself would be even > > better > > > but I forget whether the SequencedWorkerPool is aware of the originating > > > TaskRunner. > > > > It is not; it only stores the sequence token and a pointer to itself on each > > thread. > > Oh wait, I lied: it does store the shutdown behavior > (https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq...). Ok then reconstructing a SequencedTaskRunner equivalent to the original poster (i.e. same ShutdownBehaviour) sounds best to me -- and I'd argue it'd be better for the SequencedWorkerPool to have a GetCurrentSequencedTaskRunner() then for it to be constructed outside.
Message was sent while issue was closed.
On 2015/11/04 18:57:43, gab wrote: > On 2015/11/04 08:37:52, Bernhard Bauer wrote: > > On 2015/11/04 08:36:50, Bernhard Bauer wrote: > > > On 2015/11/03 22:22:33, gab wrote: > > > > On 2015/11/03 12:47:54, Bernhard Bauer wrote: > > > > > On 2015/11/02 21:51:38, danakj wrote: > > > > > > On Mon, Nov 2, 2015 at 11:11 AM, <mailto:gab@chromium.org> wrote: > > > > > > > > > > > > > On 2015/11/02 18:44:53, danakj wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > > > > > > > >> File base/threading/sequenced_task_runner_handle.cc (right): > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > > > > > > > >> base/threading/sequenced_task_runner_handle.cc:22: > > > > > > >> DCHECK(sequence_token.IsValid()); > > > > > > >> On 2015/11/02 18:42:40, gab wrote: > > > > > > >> > Actually, if the |sequence_token| is invalid: doesn't that mean > > that > > > > the > > > > > > >> current > > > > > > >> > task was posted one-off (not on a sequence)? If so, we should > still > > > > > > >> return a > > > > > > >> > valid SequenceTaskRunner (with a new token) here, right? > > > > > > > > > > The problem with this is that we would need to get a sequence token that > > is > > > > > guaranteed not to immediately start running on a different worker pool > > > thread, > > > > > otherwise we'd run the risk of violating sequentiality. > > > > > > > > You mean sequentiality from the current task? It wasn't posted to a > sequence > > > so > > > > it doesn't expect sequencing, but I guess you're saying the caller expects > > his > > > > task to be sequenced AFTER the currently running task regardless of > whether > > it > > > > is in an explicit sequence? > > > > > > > > The SequenceWorkerPool could be made to support that it feels. > > > > > > Yes, a common use case for STRH (or TTRH before that) is to run something at > a > > > later point, for example to run a callback in such a way that the caller > > doesn't > > > have to deal with reentrancy issues. Calling the callback before the > original > > > returns _on a different thread_ would probably break even more 😃 > > Right, but I'm still pretty sure STRH should be agnostic to whether the current > task itself is running as part of a sequence or not. > > As proposed below I think we want the SequencedTaskRunner to be built by the > SequencedWorkerPool itself, in which case it could special case that use-case > (i.e. kick off a new sequence which starts with the already ongoing task). > > > > > > > > > > > > > > > > Maybe I'm confused, but I thought the idea was that you'd use this > > only > > > > > > >> from > > > > > > >> within a task running on a SWP, which means it would be valid. Are > > you > > > > > > >> saying > > > > > > >> you can call this from any place and expect some SWP to pick it up? > > > > (which > > > > > > >> > > > > > > > one?) > > > > > > > > > > > > > > No, I meant the SWP also has a PostTask() entry-point (you can post > > > tasks > > > > > > > to the > > > > > > > pool without asking being tied to a Sequencetoken). > > > > > > > > > > > > > > > > > > > OIC that does sound right then, and easy to express with a test. > > > > > > > > > > > > Harder question seems to be how to we encode expected shutdown > behaviour > > > > > > into this API? > > > > > > > > > > We could add a variant of Get() that takes a ShutdownBehavior and passes > > it > > > > when > > > > > creating a new SequencedTaskRunner from the worker pool. > > > > > > > > I think we need to always inherit the ShutdownBehaviour of the originating > > > > sequence, see my reply on chromium-dev (having tasks with different > > > > ShutdownBehaviours on the same sequence doesn't make sense). In fact, > > finding > > > a > > > > way to just get back the initial SequencedTaskRunner itself would be even > > > better > > > > but I forget whether the SequencedWorkerPool is aware of the originating > > > > TaskRunner. > > > > > > It is not; it only stores the sequence token and a pointer to itself on each > > > thread. > > > > Oh wait, I lied: it does store the shutdown behavior > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq...). > > Ok then reconstructing a SequencedTaskRunner equivalent to the original poster > (i.e. same ShutdownBehaviour) sounds best to me -- and I'd argue it'd be better > for the SequencedWorkerPool to have a GetCurrentSequencedTaskRunner() then for > it to be constructed outside. Hm... so, if we are running an unsequenced task and someone calls GetCurrentSequencedTaskRunner(), so we assign a new SequenceToken to the current task -- what should the shutdown behavior be then?
Message was sent while issue was closed.
On 2015/11/04 18:57:43, gab wrote: > On 2015/11/04 08:37:52, Bernhard Bauer wrote: > > On 2015/11/04 08:36:50, Bernhard Bauer wrote: > > > On 2015/11/03 22:22:33, gab wrote: > > > > On 2015/11/03 12:47:54, Bernhard Bauer wrote: > > > > > On 2015/11/02 21:51:38, danakj wrote: > > > > > > On Mon, Nov 2, 2015 at 11:11 AM, <mailto:gab@chromium.org> wrote: > > > > > > > > > > > > > On 2015/11/02 18:44:53, danakj wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > > > > > > > >> File base/threading/sequenced_task_runner_handle.cc (right): > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > > > > > > > >> base/threading/sequenced_task_runner_handle.cc:22: > > > > > > >> DCHECK(sequence_token.IsValid()); > > > > > > >> On 2015/11/02 18:42:40, gab wrote: > > > > > > >> > Actually, if the |sequence_token| is invalid: doesn't that mean > > that > > > > the > > > > > > >> current > > > > > > >> > task was posted one-off (not on a sequence)? If so, we should > still > > > > > > >> return a > > > > > > >> > valid SequenceTaskRunner (with a new token) here, right? > > > > > > > > > > The problem with this is that we would need to get a sequence token that > > is > > > > > guaranteed not to immediately start running on a different worker pool > > > thread, > > > > > otherwise we'd run the risk of violating sequentiality. > > > > > > > > You mean sequentiality from the current task? It wasn't posted to a > sequence > > > so > > > > it doesn't expect sequencing, but I guess you're saying the caller expects > > his > > > > task to be sequenced AFTER the currently running task regardless of > whether > > it > > > > is in an explicit sequence? > > > > > > > > The SequenceWorkerPool could be made to support that it feels. > > > > > > Yes, a common use case for STRH (or TTRH before that) is to run something at > a > > > later point, for example to run a callback in such a way that the caller > > doesn't > > > have to deal with reentrancy issues. Calling the callback before the > original > > > returns _on a different thread_ would probably break even more 😃 > > Right, but I'm still pretty sure STRH should be agnostic to whether the current > task itself is running as part of a sequence or not. What happens if class A is running on a thread task runner, and grabs the STRH and posts anther method on class A to it? Will that wait for the current task to end before running? > As proposed below I think we want the SequencedTaskRunner to be built by the > SequencedWorkerPool itself, in which case it could special case that use-case > (i.e. kick off a new sequence which starts with the already ongoing task). > > > > > > > > > > > > > > > > Maybe I'm confused, but I thought the idea was that you'd use this > > only > > > > > > >> from > > > > > > >> within a task running on a SWP, which means it would be valid. Are > > you > > > > > > >> saying > > > > > > >> you can call this from any place and expect some SWP to pick it up? > > > > (which > > > > > > >> > > > > > > > one?) > > > > > > > > > > > > > > No, I meant the SWP also has a PostTask() entry-point (you can post > > > tasks > > > > > > > to the > > > > > > > pool without asking being tied to a Sequencetoken). > > > > > > > > > > > > > > > > > > > OIC that does sound right then, and easy to express with a test. > > > > > > > > > > > > Harder question seems to be how to we encode expected shutdown > behaviour > > > > > > into this API? > > > > > > > > > > We could add a variant of Get() that takes a ShutdownBehavior and passes > > it > > > > when > > > > > creating a new SequencedTaskRunner from the worker pool. > > > > > > > > I think we need to always inherit the ShutdownBehaviour of the originating > > > > sequence, see my reply on chromium-dev (having tasks with different > > > > ShutdownBehaviours on the same sequence doesn't make sense). In fact, > > finding > > > a > > > > way to just get back the initial SequencedTaskRunner itself would be even > > > better > > > > but I forget whether the SequencedWorkerPool is aware of the originating > > > > TaskRunner. > > > > > > It is not; it only stores the sequence token and a pointer to itself on each > > > thread. > > > > Oh wait, I lied: it does store the shutdown behavior > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq...). > > Ok then reconstructing a SequencedTaskRunner equivalent to the original poster > (i.e. same ShutdownBehaviour) sounds best to me -- and I'd argue it'd be better > for the SequencedWorkerPool to have a GetCurrentSequencedTaskRunner() then for > it to be constructed outside.
Message was sent while issue was closed.
Yes, that would be the desired behavior, at least for my case. We could implement it by setting the newly created sequence token on the current worker, so the pool would know not to schedule the posted task until the current one has finished. On Wed, Nov 4, 2015, 19:08 <danakj@chromium.org> wrote: > On 2015/11/04 18:57:43, gab wrote: > > On 2015/11/04 08:37:52, Bernhard Bauer wrote: > > > On 2015/11/04 08:36:50, Bernhard Bauer wrote: > > > > On 2015/11/03 22:22:33, gab wrote: > > > > > On 2015/11/03 12:47:54, Bernhard Bauer wrote: > > > > > > On 2015/11/02 21:51:38, danakj wrote: > > > > > > > On Mon, Nov 2, 2015 at 11:11 AM, <mailto:gab@chromium.org> > > wrote: > > > > > > > > > > > > > > > On 2015/11/02 18:44:53, danakj wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > > > > > > > > > >> File base/threading/sequenced_task_runner_handle.cc (right): > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1423773003/diff/160001/base/threading/sequenc... > > > > > > > > > > > > > > > >> base/threading/sequenced_task_runner_handle.cc:22: > > > > > > > >> DCHECK(sequence_token.IsValid()); > > > > > > > >> On 2015/11/02 18:42:40, gab wrote: > > > > > > > >> > Actually, if the |sequence_token| is invalid: doesn't that > > mean > > > that > > > > > the > > > > > > > >> current > > > > > > > >> > task was posted one-off (not on a sequence)? If so, we > > should > > still > > > > > > > >> return a > > > > > > > >> > valid SequenceTaskRunner (with a new token) here, right? > > > > > > > > > > > > The problem with this is that we would need to get a sequence > > token > that > > > is > > > > > > guaranteed not to immediately start running on a different worker > > pool > > > > thread, > > > > > > otherwise we'd run the risk of violating sequentiality. > > > > > > > > > > You mean sequentiality from the current task? It wasn't posted to a > > sequence > > > > so > > > > > it doesn't expect sequencing, but I guess you're saying the caller > expects > > > his > > > > > task to be sequenced AFTER the currently running task regardless of > > whether > > > it > > > > > is in an explicit sequence? > > > > > > > > > > The SequenceWorkerPool could be made to support that it feels. > > > > > > > > Yes, a common use case for STRH (or TTRH before that) is to run > > something > at > > a > > > > later point, for example to run a callback in such a way that the > > caller > > > doesn't > > > > have to deal with reentrancy issues. Calling the callback before the > > original > > > > returns _on a different thread_ would probably break even more 😃 > > > Right, but I'm still pretty sure STRH should be agnostic to whether the > current > > task itself is running as part of a sequence or not. > > What happens if class A is running on a thread task runner, and grabs the > STRH > and posts anther method on class A to it? Will that wait for the current > task to > end before running? > > > As proposed below I think we want the SequencedTaskRunner to be built by > > the > > SequencedWorkerPool itself, in which case it could special case that > > use-case > > (i.e. kick off a new sequence which starts with the already ongoing > task). > > > > > > > > > > > > > > > > > > > Maybe I'm confused, but I thought the idea was that you'd use > > this > > > only > > > > > > > >> from > > > > > > > >> within a task running on a SWP, which means it would be > > valid. > Are > > > you > > > > > > > >> saying > > > > > > > >> you can call this from any place and expect some SWP to pick > > it > up? > > > > > (which > > > > > > > >> > > > > > > > > one?) > > > > > > > > > > > > > > > > No, I meant the SWP also has a PostTask() entry-point (you > can > post > > > > tasks > > > > > > > > to the > > > > > > > > pool without asking being tied to a Sequencetoken). > > > > > > > > > > > > > > > > > > > > > > OIC that does sound right then, and easy to express with a > test. > > > > > > > > > > > > > > Harder question seems to be how to we encode expected shutdown > > behaviour > > > > > > > into this API? > > > > > > > > > > > > We could add a variant of Get() that takes a ShutdownBehavior and > passes > > > it > > > > > when > > > > > > creating a new SequencedTaskRunner from the worker pool. > > > > > > > > > > I think we need to always inherit the ShutdownBehaviour of the > originating > > > > > sequence, see my reply on chromium-dev (having tasks with different > > > > > ShutdownBehaviours on the same sequence doesn't make sense). In > > fact, > > > finding > > > > a > > > > > way to just get back the initial SequencedTaskRunner itself would > be > even > > > > better > > > > > but I forget whether the SequencedWorkerPool is aware of the > > originating > > > > > TaskRunner. > > > > > > > > It is not; it only stores the sequence token and a pointer to itself > > on > each > > > > thread. > > > > > > Oh wait, I lied: it does store the shutdown behavior > > > > > ( > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq... > ). > > > Ok then reconstructing a SequencedTaskRunner equivalent to the original > > poster > > (i.e. same ShutdownBehaviour) sounds best to me -- and I'd argue it'd be > better > > for the SequencedWorkerPool to have a GetCurrentSequencedTaskRunner() > > then for > > it to be constructed outside. > > > > https://codereview.chromium.org/1423773003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
@Dana: The single-thread version doesn't have this issue because posted task go into a queue which can only be processed after the current task is complete (whereas with the SequencedWorkerPool, they're immediately eligible to run if not on an active sequence). @Bernhard: ShutdownBehaviour of the new sequence would inherit the ShutdownBehaviour of the unsequenced task IMO (individual tasks still have a ShutdownBehaviour even if not sequenced).
Message was sent while issue was closed.
On 2015/11/04 19:26:31, gab wrote: > @Dana: The single-thread version doesn't have this issue because posted task go > into a queue which can only be processed after the current task is complete > (whereas with the SequencedWorkerPool, they're immediately eligible to run if > not on an active sequence). > > @Bernhard: ShutdownBehaviour of the new sequence would inherit the > ShutdownBehaviour of the unsequenced task IMO (individual tasks still have a > ShutdownBehaviour even if not sequenced). True! OK, I uploaded https://codereview.chromium.org/1414793009/ to continue the discussion ☺ It's a bit unfinished (in particular missing comments and tests), but it should give you an idea of what I'm planning. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
