Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414793009:
Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks. (Closed)
|
Created:
5 years, 1 month ago by Bernhard Bauer Modified:
4 years, 6 months ago CC:
chromium-reviews, vmpstr+watch_chromium.org, Ryan Sleevi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow SequencedTaskRunnerHandle::Get() while running unsequenced tasks.
If the SequencedWorkerPool is running an unsequenced task, it will
assign a new sequence token to it and use that for the returned
SequencedTaskRunner.
Committed: https://crrev.com/5d315e4b4a7cdef34b0c1e863353d03a30781217
Cr-Commit-Position: refs/heads/master@{#368078}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : x #
Total comments: 11
Patch Set 4 : tests & comments #Patch Set 5 : fix #Patch Set 6 : fix #
Total comments: 14
Patch Set 7 : review #
Total comments: 4
Patch Set 8 : review #
Total comments: 2
Patch Set 9 : remove base:: #
Total comments: 34
Patch Set 10 : review #
Total comments: 4
Patch Set 11 : review #Patch Set 12 : added comment #
Total comments: 3
Patch Set 13 : review #Patch Set 14 : moar test #
Total comments: 2
Patch Set 15 : ASSERT -> EXPECT #Patch Set 16 : sync #
Messages
Total messages: 58 (17 generated)
Description was changed from ========== Add SequencedTaskRunnerHandle::GetWithShutdownBehavior(). BUG=XXX ========== to ========== Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks. If the SequencedWorkerPool is running an unsequenced task, it will assign a new sequence token to it and use that for the returned SequencedTaskRunner. ==========
danakj@chromium.org changed reviewers: + danakj@chromium.org, rsleevi@chromium.org
https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:20: return base::ThreadTaskRunnerHandle::Get(); I'm wondering if this is a good idea if we shouldn't default to using SequencedTaskRunnerHandle::Get() instead of ThreadTaskRunnerHandle::Get(). I'd like to see that conversation resolved.. maybe it was? It didn't feel like it to me. And maybe update the guidance comment in this CL accordingly.
gab@chromium.org changed reviewers: + gab@chromium.org
The overall approach LG, let us know when comments/tests have been updated. Thanks! Gab https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (left): https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:9: #include "base/threading/sequenced_worker_pool.h" Still being used. https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:17: return task_runner; How about: // Return the SequencedTaskRunner if found or the SingleThreadedTaskRunner for the current thread otherwise. return task_runner ? task_runner : base::ThreadTaskRunnerHandle::Get(); https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:20: return base::ThreadTaskRunnerHandle::Get(); On 2015/11/05 18:52:08, danakj wrote: > I'm wondering if this is a good idea if we shouldn't default to using > SequencedTaskRunnerHandle::Get() instead of ThreadTaskRunnerHandle::Get(). > > I'd like to see that conversation resolved.. maybe it was? It didn't feel like > it to me. And maybe update the guidance comment in this CL accordingly. Hmmm? SequencedTaskRunnerHandle::Get() here would result in an infinite recursion loop. The discussion was more around whether the preference towards SequencedTaskRunnerHandle highlighted in thread_task_runner_handle.h is correct. https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.h (right): https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.h:10: #include "base/threading/sequenced_worker_pool.h" unused
https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:20: return base::ThreadTaskRunnerHandle::Get(); On 2015/11/05 23:29:18, gab wrote: > On 2015/11/05 18:52:08, danakj wrote: > > I'm wondering if this is a good idea if we shouldn't default to using > > SequencedTaskRunnerHandle::Get() instead of ThreadTaskRunnerHandle::Get(). > > > > I'd like to see that conversation resolved.. maybe it was? It didn't feel like > > it to me. And maybe update the guidance comment in this CL accordingly. > > Hmmm? SequencedTaskRunnerHandle::Get() here would result in an infinite > recursion loop. The discussion was more around whether the preference towards > SequencedTaskRunnerHandle highlighted in thread_task_runner_handle.h is correct. Oh I meant, if this isn't a general "always use that", then it's more of a "I know/expect to be part of a sequenced worker pool" and maybe this should return null, instead of ThreadTaskRunnerHandle, if it's not.
https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:20: return base::ThreadTaskRunnerHandle::Get(); On 2015/11/05 23:31:15, danakj wrote: > On 2015/11/05 23:29:18, gab wrote: > > On 2015/11/05 18:52:08, danakj wrote: > > > I'm wondering if this is a good idea if we shouldn't default to using > > > SequencedTaskRunnerHandle::Get() instead of ThreadTaskRunnerHandle::Get(). > > > > > > I'd like to see that conversation resolved.. maybe it was? It didn't feel > like > > > it to me. And maybe update the guidance comment in this CL accordingly. > > > > Hmmm? SequencedTaskRunnerHandle::Get() here would result in an infinite > > recursion loop. The discussion was more around whether the preference towards > > SequencedTaskRunnerHandle highlighted in thread_task_runner_handle.h is > correct. > > Oh I meant, if this isn't a general "always use that", then it's more of a "I > know/expect to be part of a sequenced worker pool" and maybe this should return > null, instead of ThreadTaskRunnerHandle, if it's not. Oh I see. I don't think so. I think returning a SingleThreadedTaskRunner here as a fallback is always the right thing to do. The debate is more whether to prefer the loose API or the strict API from the caller's standpoint I think.
bauerb@chromium.org changed reviewers: - rsleevi@chromium.org
Tests and comments added. PTAL, thanks! https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:17: return task_runner; On 2015/11/05 23:29:18, gab wrote: > How about: > > // Return the SequencedTaskRunner if found or the SingleThreadedTaskRunner for > the current thread otherwise. > return task_runner ? task_runner : base::ThreadTaskRunnerHandle::Get(); Done, although I had to add an additional constructor call to make the ?: work. https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.h (right): https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.h:10: #include "base/threading/sequenced_worker_pool.h" On 2015/11/05 23:29:18, gab wrote: > unused Done.
https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:17: return task_runner; On 2015/11/10 17:12:06, Bernhard Bauer wrote: > On 2015/11/05 23:29:18, gab wrote: > > How about: > > > > // Return the SequencedTaskRunner if found or the SingleThreadedTaskRunner for > > the current thread otherwise. > > return task_runner ? task_runner : base::ThreadTaskRunnerHandle::Get(); > > Done, although I had to add an additional constructor call to make the ?: work. Oh interesting, without this the implicit conversion works but not with it? https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_task_runner_handle_unittest.cc:26: static void GetTaskRunner(const Closure& callback) { A "Get" method that returns void feels weird to me. https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_task_runner_handle_unittest.cc:68: TEST_F(SequencedTaskRunnerHandleTest, FromUnsequencedTask) { Did you confirm that this test failed prior to the product side changes of this CL? https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:697: DCHECK(!found->second->task_sequence_token().IsValid()); I don't see code unsetting this when the task is done? Will this truly be invalid the second time around on the same thread for another sequence? https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1205: scoped_refptr<SequencedWorkerPool> pool = worker->worker_pool(); Move |pool| to 1213 (above DCHECK) as it's not needed before then -- and probably also add a line break after the '}' below. https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:167: // so that the returned SequencedTaskRunner is guaranteed to runs tasks after s/runs/run https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:907: void VerifySequencedTaskRunner( IsSameSequence() or something? (+ a commment?) -- i.e., it's not clear to me from the name what this is meant to do without reading the calling code. https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:912: void GetCurrentSequencedTaskRunner( Again, a "Get" method that returns void feels weird to me. Maybe, VerifyCurrentTaskRunner?
https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1414793009/diff/40001/base/threading/sequence... base/threading/sequenced_task_runner_handle.cc:17: return task_runner; On 2015/11/10 19:54:18, gab wrote: > On 2015/11/10 17:12:06, Bernhard Bauer wrote: > > On 2015/11/05 23:29:18, gab wrote: > > > How about: > > > > > > // Return the SequencedTaskRunner if found or the SingleThreadedTaskRunner > for > > > the current thread otherwise. > > > return task_runner ? task_runner : base::ThreadTaskRunnerHandle::Get(); > > > > Done, although I had to add an additional constructor call to make the ?: > work. > > Oh interesting, without this the implicit conversion works but not with it? Yes, I think the two alternatives in a conditional operator have to be the same type. https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_task_runner_handle_unittest.cc:26: static void GetTaskRunner(const Closure& callback) { On 2015/11/10 19:54:18, gab wrote: > A "Get" method that returns void feels weird to me. Done. https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_task_runner_handle_unittest.cc:68: TEST_F(SequencedTaskRunnerHandleTest, FromUnsequencedTask) { On 2015/11/10 19:54:18, gab wrote: > Did you confirm that this test failed prior to the product side changes of this > CL? Yes: [ RUN ] SequencedTaskRunnerHandleTest.FromUnsequencedTask ../../base/threading/sequenced_task_runner_handle_unittest.cc:27: Failure Value of: SequencedTaskRunnerHandle::IsSet() Actual: false Expected: true (It hangs after that, because I was too lazy to add something that would call the callback in case of failure... 😃) https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:697: DCHECK(!found->second->task_sequence_token().IsValid()); On 2015/11/10 19:54:18, gab wrote: > I don't see code unsetting this when the task is done? Will this truly be > invalid the second time around on the same thread for another sequence? This is set before the next task is run, so it will have that task's sequence token then. https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1205: scoped_refptr<SequencedWorkerPool> pool = worker->worker_pool(); On 2015/11/10 19:54:19, gab wrote: > Move |pool| to 1213 (above DCHECK) as it's not needed before then -- and > probably also add a line break after the '}' below. I do need it in line 1210, so I could only move it down two lines. https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:167: // so that the returned SequencedTaskRunner is guaranteed to runs tasks after On 2015/11/10 19:54:19, gab wrote: > s/runs/run Done. https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:907: void VerifySequencedTaskRunner( On 2015/11/10 19:54:19, gab wrote: > IsSameSequence() or something? (+ a commment?) -- i.e., it's not clear to me > from the name what this is meant to do without reading the calling code. Done -- tried to make the name as descriptive as possible. https://codereview.chromium.org/1414793009/diff/100001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:912: void GetCurrentSequencedTaskRunner( On 2015/11/10 19:54:19, gab wrote: > Again, a "Get" method that returns void feels weird to me. > > Maybe, VerifyCurrentTaskRunner? Done.
lgtm w/ 2 requests, thanks! https://codereview.chromium.org/1414793009/diff/120001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/120001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:949: FROM_HERE, base::Bind(&VerifyCurrentSequencedTaskRunner, task_runner_2)); Would be nice to also verify inequality (i.e. task_runner_1 vs task_runner_2)? https://codereview.chromium.org/1414793009/diff/120001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:954: pool()->FlushForTesting(); This seems risky as if the PostTask on 928 posts to an incorrect sequence somehow (i.e. not on this pool()) then the test would finish without catching this. Feels some kind of WaitableEvent like in your other test would preferable to confirm that the post chain is truly complete.
Thanks for the review! https://codereview.chromium.org/1414793009/diff/120001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/120001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:949: FROM_HERE, base::Bind(&VerifyCurrentSequencedTaskRunner, task_runner_2)); On 2015/11/12 01:33:56, gab wrote: > Would be nice to also verify inequality (i.e. task_runner_1 vs task_runner_2)? Done. https://codereview.chromium.org/1414793009/diff/120001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:954: pool()->FlushForTesting(); On 2015/11/12 01:33:56, gab wrote: > This seems risky as if the PostTask on 928 posts to an incorrect sequence > somehow (i.e. not on this pool()) then the test would finish without catching > this. > > Feels some kind of WaitableEvent like in your other test would preferable to > confirm that the post chain is truly complete. Done.
Thanks, lgtm++ https://codereview.chromium.org/1414793009/diff/140001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/140001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:950: Closure signal = base::Bind(&WaitableEvent::Signal, base::Unretained(&event)); Either use base:: namespace or don't (but here you do for Bind and not for Closure which is weird).
Dana, could you TAL, P? https://codereview.chromium.org/1414793009/diff/140001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/140001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:950: Closure signal = base::Bind(&WaitableEvent::Signal, base::Unretained(&event)); On 2015/11/12 14:23:25, gab wrote: > Either use base:: namespace or don't (but here you do for Bind and not for > Closure which is weird). Right! I am already in the base namespace, but I'm so used to always adding base:: 😃
> If the SequencedWorkerPool is running an unsequenced task, it will assign a new sequence token to it and use that for the returned SequencedTaskRunner. Wrap at 72 cols please. http://chris.beams.io/posts/git-commit/#wrap-72
Description was changed from ========== Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks. If the SequencedWorkerPool is running an unsequenced task, it will assign a new sequence token to it and use that for the returned SequencedTaskRunner. ========== to ========== Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks. If the SequencedWorkerPool is running an unsequenced task, it will assign a new sequence token to it and use that for the returned SequencedTaskRunner. ==========
On 2015/11/17 21:30:39, danakj (behind on reviews) wrote: > > If the SequencedWorkerPool is running an unsequenced task, it will assign a > new sequence token to it and use that for the returned SequencedTaskRunner. > > Wrap at 72 cols please. http://chris.beams.io/posts/git-commit/#wrap-72 Done.
I haz questions! https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_task_runner_handle.cc:19: return task_runner ? task_runner : scoped_refptr<SequencedTaskRunner>( I liked without the ?: better >__> https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:829: // still works. The task.sequence_token_id is no longer correct after calling run here sometimes? What kind of problems might that make? Does that break this comment? The SequencedTask destructor is empty, what checking is it talking about? https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1207: WorkerShutdown shutdown_behavior = worker->task_shutdown_behavior(); Can you DCHECK that is_processing_task() is true? https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1210: pool->inner_->SetRunningTaskInfoForCurrentThread(sequence_token, Why do you do this? Now the thread is left in a state where it's bound to this sequence after calling this. Normally we reset it to no sequence when you finish a task. What will clean that up? What is going to use what you're setting here? You're not going to Run() a task here. https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1215: return new SequencedWorkerPoolSequencedTaskRunner(pool, sequence_token, can you move() the pool smart pointer? no need to keep a ref here. https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:189: static SequenceToken GetSequenceToken(); can you move this beside other static methods (GetSTForCurrentThread) maybe we could name this better to differentiate it more? GetNewSequenceToken? https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:910: const scoped_refptr<SequencedTaskRunner>& task_runner, nit: take smart pointers by value if you want to hold a reference. take T* if you don't want a reference. i think you want SequencedTaskRunner* here? https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:919: scoped_refptr<SequencedTaskRunner> expected_task_runner, and here? https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:937: expected_task_runner, expected_equal, callback)); nit: move() the task runner smart pointer, no need to keep a ref here. but probably you don't want a refptr at all https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:956: task_runner_1, true, signal)); .get() on all these, to pass the runner pointer. we'll wait for the tasks to run on the next line so no need to introduce ref counting.
https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_task_runner_handle.cc:19: return task_runner ? task_runner : scoped_refptr<SequencedTaskRunner>( On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > I liked without the ?: better >__> FWIW, I agree, so I changed it back to two return statements for now. Gab, do you feel strongly about this? https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:829: // still works. On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > The task.sequence_token_id is no longer correct after calling run here > sometimes? What kind of problems might that make? Does that break this comment? > The SequencedTask destructor is empty, what checking is it talking about? I think this is referring to the fact that we might drop the last reference to the previous task (as in Closure, not SequencedTask), which would then destroy it and its parameters, so if any of those would have a sequence check in its destructor, it would continue to work. https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1207: WorkerShutdown shutdown_behavior = worker->task_shutdown_behavior(); On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > Can you DCHECK that is_processing_task() is true? task_shutdown_behavior() already does that. Alternatively, we could put the sequence token and shutdown behavior into a struct and keep a scoped_ptr to it that we reset in reset_running_task_info()? https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1210: pool->inner_->SetRunningTaskInfoForCurrentThread(sequence_token, On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > Why do you do this? Now the thread is left in a state where it's bound to this > sequence after calling this. Normally we reset it to no sequence when you finish > a task. What will clean that up? What is going to use what you're setting here? > You're not going to Run() a task here. I do this to make sure that a task posted to the new SequencedTaskRunner will not start running immediately on another thread... but now that you mention it, I did forget to mark the sequence token as in use, so I fixed that. We still need to set these so that calling this twice in a row will return the same sequence. Once the task is done, the task info will be reset (the old sequence token will stay around, but |is_processing_task_| will be false, which guards all accesses; see also my comment above). https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1215: return new SequencedWorkerPoolSequencedTaskRunner(pool, sequence_token, On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > can you move() the pool smart pointer? no need to keep a ref here. Done. https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.h:189: static SequenceToken GetSequenceToken(); On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > can you move this beside other static methods (GetSTForCurrentThread) > > maybe we could name this better to differentiate it more? GetNewSequenceToken? Moved it. The method is currently used in 67 places according to codesearch, so I added a TODO for now to rename it. https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:910: const scoped_refptr<SequencedTaskRunner>& task_runner, On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > nit: take smart pointers by value if you want to hold a reference. take T* if > you don't want a reference. > > i think you want SequencedTaskRunner* here? Done. https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:919: scoped_refptr<SequencedTaskRunner> expected_task_runner, On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > and here? But here I do want to take a reference (and then pass it to the closure below), no? https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:937: expected_task_runner, expected_equal, callback)); On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > nit: move() the task runner smart pointer, no need to keep a ref here. > > but probably you don't want a refptr at all Moving the scoped_refptr (see above). https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:956: task_runner_1, true, signal)); On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > .get() on all these, to pass the runner pointer. we'll wait for the tasks to run > on the next line so no need to introduce ref counting. Apparently, if the first argument to a Closure is refcounted, it needs to be in a scoped_refptr, otherwise I get a static assertion failure (https://code.google.com/p/chromium/codesearch#chromium/src/base/bind.h&sq=pac...).
https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:956: task_runner_1, true, signal)); On 2015/11/18 13:15:50, Bernhard Bauer wrote: > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > .get() on all these, to pass the runner pointer. we'll wait for the tasks to > run > > on the next line so no need to introduce ref counting. > > Apparently, if the first argument to a Closure is refcounted, it needs to be in > a scoped_refptr, otherwise I get a static assertion failure > (https://code.google.com/p/chromium/codesearch#chromium/src/base/bind.h&sq=pac...). Unless you use base::Unretained, I thought?
https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_task_runner_handle.cc:19: return task_runner ? task_runner : scoped_refptr<SequencedTaskRunner>( On 2015/11/18 13:15:50, Bernhard Bauer wrote: > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > I liked without the ?: better >__> > > FWIW, I agree, so I changed it back to two return statements for now. Gab, do > you feel strongly about this? Nope, either way is fine (would have preferred this if it wasn't for the ugly cast). https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:956: task_runner_1, true, signal)); On 2015/11/18 19:06:31, danakj (behind on reviews) wrote: > On 2015/11/18 13:15:50, Bernhard Bauer wrote: > > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > > .get() on all these, to pass the runner pointer. we'll wait for the tasks to > > run > > > on the next line so no need to introduce ref counting. > > > > Apparently, if the first argument to a Closure is refcounted, it needs to be > in > > a scoped_refptr, otherwise I get a static assertion failure > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/bind.h&sq=pac...). > > Unless you use base::Unretained, I thought? Right. https://codereview.chromium.org/1414793009/diff/180001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/180001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:831: task.sequence_token_id = this_worker->task_sequence_token().id_; Why does this matter? Isn't the task's sequence_token_id only useful for it to be picked up by |this_worker| in this method while the task is running (and therefore irrelevant after)?
https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:956: task_runner_1, true, signal)); On 2015/11/18 22:37:33, gab wrote: > On 2015/11/18 19:06:31, danakj (behind on reviews) wrote: > > On 2015/11/18 13:15:50, Bernhard Bauer wrote: > > > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > > > .get() on all these, to pass the runner pointer. we'll wait for the tasks > to > > > run > > > > on the next line so no need to introduce ref counting. > > > > > > Apparently, if the first argument to a Closure is refcounted, it needs to be > > in > > > a scoped_refptr, otherwise I get a static assertion failure > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/bind.h&sq=pac...). > > > > Unless you use base::Unretained, I thought? > > Right. So... you want me to do that then? Done (although given the choice I personally would prefer to trade the readability of using |task_runner_n| as it is in exchange for the overhead of refcounting). https://codereview.chromium.org/1414793009/diff/180001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/180001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:831: task.sequence_token_id = this_worker->task_sequence_token().id_; On 2015/11/18 22:37:33, gab wrote: > Why does this matter? Isn't the task's sequence_token_id only useful for it to > be picked up by |this_worker| in this method while the task is running (and > therefore irrelevant after)? DidRunWorkerTask() below removes the sequence token ID from the set of currently running sequences that is used to prevent multiple sequences from running at the same time. Without this, the new sequence token ID would stay in that set, so the sequence would never run anymore.
lgtm3 :-) https://codereview.chromium.org/1414793009/diff/180001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/180001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:831: task.sequence_token_id = this_worker->task_sequence_token().id_; On 2015/11/19 10:15:12, Bernhard Bauer wrote: > On 2015/11/18 22:37:33, gab wrote: > > Why does this matter? Isn't the task's sequence_token_id only useful for it to > > be picked up by |this_worker| in this method while the task is running (and > > therefore irrelevant after)? > > DidRunWorkerTask() below removes the sequence token ID from the set of currently > running sequences that is used to prevent multiple sequences from running at the > same time. Without this, the new sequence token ID would stay in that set, so > the sequence would never run anymore. I see, can you highlight that in the comment as well?
On 2015/11/19 14:34:37, gab wrote: > lgtm3 :-) Oops that doesn't parse... LGTM w/ comment > > https://codereview.chromium.org/1414793009/diff/180001/base/threading/sequenc... > File base/threading/sequenced_worker_pool.cc (right): > > https://codereview.chromium.org/1414793009/diff/180001/base/threading/sequenc... > base/threading/sequenced_worker_pool.cc:831: task.sequence_token_id = > this_worker->task_sequence_token().id_; > On 2015/11/19 10:15:12, Bernhard Bauer wrote: > > On 2015/11/18 22:37:33, gab wrote: > > > Why does this matter? Isn't the task's sequence_token_id only useful for it > to > > > be picked up by |this_worker| in this method while the task is running (and > > > therefore irrelevant after)? > > > > DidRunWorkerTask() below removes the sequence token ID from the set of > currently > > running sequences that is used to prevent multiple sequences from running at > the > > same time. Without this, the new sequence token ID would stay in that set, so > > the sequence would never run anymore. > > I see, can you highlight that in the comment as well?
https://codereview.chromium.org/1414793009/diff/180001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/180001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:831: task.sequence_token_id = this_worker->task_sequence_token().id_; On 2015/11/19 14:34:36, gab wrote: > On 2015/11/19 10:15:12, Bernhard Bauer wrote: > > On 2015/11/18 22:37:33, gab wrote: > > > Why does this matter? Isn't the task's sequence_token_id only useful for it > to > > > be picked up by |this_worker| in this method while the task is running (and > > > therefore irrelevant after)? > > > > DidRunWorkerTask() below removes the sequence token ID from the set of > currently > > running sequences that is used to prevent multiple sequences from running at > the > > same time. Without this, the new sequence token ID would stay in that set, so > > the sequence would never run anymore. > > I see, can you highlight that in the comment as well? Done.
https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:956: task_runner_1, true, signal)); On 2015/11/19 10:15:12, Bernhard Bauer wrote: > On 2015/11/18 22:37:33, gab wrote: > > On 2015/11/18 19:06:31, danakj (behind on reviews) wrote: > > > On 2015/11/18 13:15:50, Bernhard Bauer wrote: > > > > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > > > > .get() on all these, to pass the runner pointer. we'll wait for the > tasks > > to > > > > run > > > > > on the next line so no need to introduce ref counting. > > > > > > > > Apparently, if the first argument to a Closure is refcounted, it needs to > be > > > in > > > > a scoped_refptr, otherwise I get a static assertion failure > > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/bind.h&sq=pac...). > > > > > > Unless you use base::Unretained, I thought? > > > > Right. > > So... you want me to do that then? Done (although given the choice I personally > would prefer to trade the readability of using |task_runner_n| as it is in > exchange for the overhead of refcounting). Yes please, it's not the overhead I'm worried about but "own it in as few places as possible". So if there's no reason to pass refs around then don't.
On 2015/11/19 18:49:08, danakj (behind on reviews) wrote: > Yes please, it's not the overhead I'm worried about but "own it in as few places > as possible". So if there's no reason to pass refs around then don't. Gotcha. Well, done 😃
On 2015/11/19 19:42:00, Bernhard Bauer wrote: > On 2015/11/19 18:49:08, danakj (behind on reviews) wrote: > > Yes please, it's not the overhead I'm worried about but "own it in as few > places > > as possible". So if there's no reason to pass refs around then don't. > > Gotcha. Well, done 😃 Friendly ping?
Thanks! I have a few comment/test requests. https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:829: // still works. On 2015/11/18 13:15:50, Bernhard Bauer wrote: > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > The task.sequence_token_id is no longer correct after calling run here > > sometimes? What kind of problems might that make? Does that break this > comment? > > The SequencedTask destructor is empty, what checking is it talking about? > > I think this is referring to the fact that we might drop the last reference to > the previous task (as in Closure, not SequencedTask), which would then destroy > it and its parameters, so if any of those would have a sequence check in its > destructor, it would continue to work. Can you add a unit test that uses SequenceChecker in its destructor and is posted from an unsequenced task? We should maintain this if people change this class or SequenceChecker. I'm trying to see why it would work, it looks like because it would have previously seen there was no sequence, so the check in the destructor would use thread_checker_ instead of comparing sequence_token_. And because we destroy the closure after running the task on the same thread, the thread checker should pass in the same circumstances it did before this patch. Is that what you see as well? https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1207: WorkerShutdown shutdown_behavior = worker->task_shutdown_behavior(); On 2015/11/18 13:15:50, Bernhard Bauer wrote: > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > Can you DCHECK that is_processing_task() is true? > > task_shutdown_behavior() already does that. Alternatively, we could put the > sequence token and shutdown behavior into a struct and keep a scoped_ptr to it > that we reset in reset_running_task_info()? Can you add a comment at the !worker early out explaining why if worker exists, we know we're inside/processing a sequenced task? (Cuz worker is only non-null inside WorkerPool::Run(), which will run sequenced tasks and does not call GetSequencedTaskRunnerForCurrentThread itself?) https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1210: pool->inner_->SetRunningTaskInfoForCurrentThread(sequence_token, On 2015/11/18 13:15:50, Bernhard Bauer wrote: > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > Why do you do this? Now the thread is left in a state where it's bound to this > > sequence after calling this. Normally we reset it to no sequence when you > finish > > a task. What will clean that up? What is going to use what you're setting > here? > > You're not going to Run() a task here. > > I do this to make sure that a task posted to the new SequencedTaskRunner will > not start running immediately on another thread... but now that you mention it, > I did forget to mark the sequence token as in use, so I fixed that. We still > need to set these so that calling this twice in a row will return the same > sequence. > > Once the task is done, the task info will be reset (the old sequence token will > stay around, but |is_processing_task_| will be false, which guards all accesses; > see also my comment above). Can you encode this into a comment in here? And can you add a unit test that would fail without your fix mentioned here? https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:919: scoped_refptr<SequencedTaskRunner> expected_task_runner, On 2015/11/18 13:15:50, Bernhard Bauer wrote: > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > and here? > > But here I do want to take a reference (and then pass it to the closure below), > no? You don't need a ref here to pass to the closure. The caller of this is waiting until the |callback| is run, at which point these will not be referring to the task runner any more. This could totally be a T* (which was why I wanted you to pass a T* in the Bind() of this function). However.. I noticed that on line 928 you're getting a scoped_refptr back from GetSTRForCurrentThread(). There's nothing keeping that one alive, so I guess you do actually need to pass a scoped_refptr. Given that, you should probably undo passing a raw pointer to the Bind of this function, cuz it's just going into a scoped_refptr anyways. Sorry for being confused here. https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:956: task_runner_1, true, signal)); On 2015/11/19 18:49:07, danakj (behind on reviews) wrote: > On 2015/11/19 10:15:12, Bernhard Bauer wrote: > > On 2015/11/18 22:37:33, gab wrote: > > > On 2015/11/18 19:06:31, danakj (behind on reviews) wrote: > > > > On 2015/11/18 13:15:50, Bernhard Bauer wrote: > > > > > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > > > > > .get() on all these, to pass the runner pointer. we'll wait for the > > tasks > > > to > > > > > run > > > > > > on the next line so no need to introduce ref counting. > > > > > > > > > > Apparently, if the first argument to a Closure is refcounted, it needs > to > > be > > > > in > > > > > a scoped_refptr, otherwise I get a static assertion failure > > > > > > > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/bind.h&sq=pac...). > > > > > > > > Unless you use base::Unretained, I thought? > > > > > > Right. > > > > So... you want me to do that then? Done (although given the choice I > personally > > would prefer to trade the readability of using |task_runner_n| as it is in > > exchange for the overhead of refcounting). > > Yes please, it's not the overhead I'm worried about but "own it in as few places > as possible". So if there's no reason to pass refs around then don't. I was wrong :( Didn't see the GetSTRForCurrentThread(), which does need to be kept alive. Simplest way to do that is to pass a refptr here too. https://codereview.chromium.org/1414793009/diff/220001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/220001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:832: task.sequence_token_id = this_worker->task_sequence_token().id_; Is this line captured in any unit tests? https://codereview.chromium.org/1414793009/diff/220001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/220001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:919: scoped_refptr<SequencedTaskRunner> expected_task_runner, Sorry I meant you can take a SequencedTaskRunner* here, it doesn't need to keep it alive/add a ref. That was why I meant to pass a T* instead of a scoped_refptr<T> at the Bind() calls.
Patchset #13 (id:240001) has been deleted
Patchset #13 (id:260001) has been deleted
Sorry for the delay. PTAL, and let me know if I've missed anything, thanks! https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:829: // still works. On 2015/11/20 19:18:43, danakj (behind on reviews) wrote: > On 2015/11/18 13:15:50, Bernhard Bauer wrote: > > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > > The task.sequence_token_id is no longer correct after calling run here > > > sometimes? What kind of problems might that make? Does that break this > > comment? > > > The SequencedTask destructor is empty, what checking is it talking about? > > > > I think this is referring to the fact that we might drop the last reference to > > the previous task (as in Closure, not SequencedTask), which would then destroy > > it and its parameters, so if any of those would have a sequence check in its > > destructor, it would continue to work. > > Can you add a unit test that uses SequenceChecker in its destructor and is > posted from an unsequenced task? We should maintain this if people change this > class or SequenceChecker. Done! I verified that the test fails if the task info is reset before. > I'm trying to see why it would work, it looks like because it would have > previously seen there was no sequence, so the check in the destructor would use > thread_checker_ instead of comparing sequence_token_. And because we destroy the > closure after running the task on the same thread, the thread checker should > pass in the same circumstances it did before this patch. Is that what you see as > well? Yup. A SequenceChecker will fail on an unsequenced task, so changing it so be sequenced will not break anything. It also won't cause false negatives, because the sequence will be brand new, so a previously existing SequenceChecker can't have used it. https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1207: WorkerShutdown shutdown_behavior = worker->task_shutdown_behavior(); On 2015/11/20 19:18:43, danakj (behind on reviews) wrote: > On 2015/11/18 13:15:50, Bernhard Bauer wrote: > > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > > Can you DCHECK that is_processing_task() is true? > > > > task_shutdown_behavior() already does that. Alternatively, we could put the > > sequence token and shutdown behavior into a struct and keep a scoped_ptr to it > > that we reset in reset_running_task_info()? > > Can you add a comment at the !worker early out explaining why if worker exists, > we know we're inside/processing a sequenced task? (Cuz worker is only non-null > inside WorkerPool::Run(), which will run sequenced tasks and does not call > GetSequencedTaskRunnerForCurrentThread itself?) Done, modulo s/sequenced //, because a worker thread will run both sequenced and unsequenced tasks. If it is running a sequenced task, we can just return a SequencedWorkerPoolSequencedTaskRunner for the current sequence token; if it is running an unsequenced task, we have to do the dance below. https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1210: pool->inner_->SetRunningTaskInfoForCurrentThread(sequence_token, On 2015/11/20 19:18:43, danakj (behind on reviews) wrote: > On 2015/11/18 13:15:50, Bernhard Bauer wrote: > > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > > Why do you do this? Now the thread is left in a state where it's bound to > this > > > sequence after calling this. Normally we reset it to no sequence when you > > finish > > > a task. What will clean that up? What is going to use what you're setting > > here? > > > You're not going to Run() a task here. > > > > I do this to make sure that a task posted to the new SequencedTaskRunner will > > not start running immediately on another thread... but now that you mention > it, > > I did forget to mark the sequence token as in use, so I fixed that. We still > > need to set these so that calling this twice in a row will return the same > > sequence. > > > > Once the task is done, the task info will be reset (the old sequence token > will > > stay around, but |is_processing_task_| will be false, which guards all > accesses; > > see also my comment above). > > Can you encode this into a comment in here? Done. > And can you add a unit test that would fail without your fix mentioned here? I tried that, but it's tricky, because I can't really verify that a task posted to the new SequencedTaskRunner will *not* immediately start running -- I can fail if it does, but if the tasks happen to run in sequence, it doesn't necessarily mean that wasn't just a coincidence. I could post a task to the new SequencedTaskRunner and verify that it tries to signal an existing thread that there is work instead of starting a new thread, which indicates that the task isn't runnable yet, but that also seems like it relies on a bunch of implementation details of the SWP... Do you have any better ideas? https://codereview.chromium.org/1414793009/diff/220001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/220001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:832: task.sequence_token_id = this_worker->task_sequence_token().id_; On 2015/11/20 19:18:43, danakj (behind on reviews) wrote: > Is this line captured in any unit tests? Yes, SequencedWorkerPoolTest.GetSequencedTaskRunnerForCurrentThread will hang without this line, because the next task with this sequence token could never run (as it's never removed from the set).
Friendly ping?
LGTM thanks for all the changes, one last thought about testing https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... base/threading/sequenced_worker_pool.cc:1210: pool->inner_->SetRunningTaskInfoForCurrentThread(sequence_token, On 2015/12/10 16:09:58, Bernhard Bauer wrote: > On 2015/11/20 19:18:43, danakj (behind on reviews) wrote: > > On 2015/11/18 13:15:50, Bernhard Bauer wrote: > > > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > > > Why do you do this? Now the thread is left in a state where it's bound to > > this > > > > sequence after calling this. Normally we reset it to no sequence when you > > > finish > > > > a task. What will clean that up? What is going to use what you're setting > > > here? > > > > You're not going to Run() a task here. > > > > > > I do this to make sure that a task posted to the new SequencedTaskRunner > will > > > not start running immediately on another thread... but now that you mention > > it, > > > I did forget to mark the sequence token as in use, so I fixed that. We still > > > need to set these so that calling this twice in a row will return the same > > > sequence. > > > > > > Once the task is done, the task info will be reset (the old sequence token > > will > > > stay around, but |is_processing_task_| will be false, which guards all > > accesses; > > > see also my comment above). > > > > Can you encode this into a comment in here? > > Done. > > > And can you add a unit test that would fail without your fix mentioned here? > > I tried that, but it's tricky, because I can't really verify that a task posted > to the new SequencedTaskRunner will *not* immediately start running -- I can > fail if it does, but if the tasks happen to run in sequence, it doesn't > necessarily mean that wasn't just a coincidence. > > I could post a task to the new SequencedTaskRunner and verify that it tries to > signal an existing thread that there is work instead of starting a new thread, > which indicates that the task isn't runnable yet, but that also seems like it > relies on a bunch of implementation details of the SWP... Do you have any better > ideas? What about just calling it twice and making sure you get the same sequence token and that IsSequenceTokenRunnable() is true?
On 2015/12/16 21:53:32, danakj (behind on reviews) wrote: > LGTM thanks for all the changes, one last thought about testing > > https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... > File base/threading/sequenced_worker_pool.cc (right): > > https://codereview.chromium.org/1414793009/diff/160001/base/threading/sequenc... > base/threading/sequenced_worker_pool.cc:1210: > pool->inner_->SetRunningTaskInfoForCurrentThread(sequence_token, > On 2015/12/10 16:09:58, Bernhard Bauer wrote: > > On 2015/11/20 19:18:43, danakj (behind on reviews) wrote: > > > On 2015/11/18 13:15:50, Bernhard Bauer wrote: > > > > On 2015/11/17 22:03:39, danakj (behind on reviews) wrote: > > > > > Why do you do this? Now the thread is left in a state where it's bound > to > > > this > > > > > sequence after calling this. Normally we reset it to no sequence when > you > > > > finish > > > > > a task. What will clean that up? What is going to use what you're > setting > > > > here? > > > > > You're not going to Run() a task here. > > > > > > > > I do this to make sure that a task posted to the new SequencedTaskRunner > > will > > > > not start running immediately on another thread... but now that you > mention > > > it, > > > > I did forget to mark the sequence token as in use, so I fixed that. We > still > > > > need to set these so that calling this twice in a row will return the same > > > > sequence. > > > > > > > > Once the task is done, the task info will be reset (the old sequence token > > > will > > > > stay around, but |is_processing_task_| will be false, which guards all > > > accesses; > > > > see also my comment above). > > > > > > Can you encode this into a comment in here? > > > > Done. > > > > > And can you add a unit test that would fail without your fix mentioned here? > > > > I tried that, but it's tricky, because I can't really verify that a task > posted > > to the new SequencedTaskRunner will *not* immediately start running -- I can > > fail if it does, but if the tasks happen to run in sequence, it doesn't > > necessarily mean that wasn't just a coincidence. > > > > I could post a task to the new SequencedTaskRunner and verify that it tries to > > signal an existing thread that there is work instead of starting a new thread, > > which indicates that the task isn't runnable yet, but that also seems like it > > relies on a bunch of implementation details of the SWP... Do you have any > better > > ideas? > > What about just calling it twice and making sure you get the same sequence token > and that IsSequenceTokenRunnable() is true? Yeah, that works (although I need to check that IsSequenceTokenRunnable() is _false_, because we are already running the sequence). I had to add a new method to SWP to expose this. I've also split off an additional method from VerifyCurrentSequencedTaskRunner() for checks when running an unsequenced task. That in turn meant that I didn't have to take a scoped_refptr as an argument, so I've rewritten the methods to take more raw pointers ☺ If you want to take another look, go ahead, otherwise I'm going to submit this soon-ish in the interest of getting this landed (but I'm happy to do followups if you find something later). Thanks!
LGTM https://codereview.chromium.org/1414793009/diff/300001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/300001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:943: ASSERT_TRUE(sequence_token.IsValid()); nit: i generally prefer to always EXPECT rather than ASSERT unless it's guarding a crash on the next line. that way there's a chance you get some more data from furthre fails
https://codereview.chromium.org/1414793009/diff/300001/base/threading/sequenc... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/1414793009/diff/300001/base/threading/sequenc... base/threading/sequenced_worker_pool_unittest.cc:943: ASSERT_TRUE(sequence_token.IsValid()); On 2015/12/17 23:49:51, danakj (behind on reviews) wrote: > nit: i generally prefer to always EXPECT rather than ASSERT unless it's guarding > a crash on the next line. that way there's a chance you get some more data from > furthre fails Done.
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, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1414793009/#ps320001 (title: "ASSERT -> EXPECT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414793009/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414793009/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bauerb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414793009/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414793009/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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/1414793009/#ps340001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414793009/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414793009/340001
Message was sent while issue was closed.
Description was changed from ========== Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks. If the SequencedWorkerPool is running an unsequenced task, it will assign a new sequence token to it and use that for the returned SequencedTaskRunner. ========== to ========== Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks. If the SequencedWorkerPool is running an unsequenced task, it will assign a new sequence token to it and use that for the returned SequencedTaskRunner. ==========
Message was sent while issue was closed.
Committed patchset #16 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks. If the SequencedWorkerPool is running an unsequenced task, it will assign a new sequence token to it and use that for the returned SequencedTaskRunner. ========== to ========== Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks. If the SequencedWorkerPool is running an unsequenced task, it will assign a new sequence token to it and use that for the returned SequencedTaskRunner. Committed: https://crrev.com/5d315e4b4a7cdef34b0c1e863353d03a30781217 Cr-Commit-Position: refs/heads/master@{#368078} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/5d315e4b4a7cdef34b0c1e863353d03a30781217 Cr-Commit-Position: refs/heads/master@{#368078}
Message was sent while issue was closed.
Description was changed from ========== Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks. If the SequencedWorkerPool is running an unsequenced task, it will assign a new sequence token to it and use that for the returned SequencedTaskRunner. Committed: https://crrev.com/5d315e4b4a7cdef34b0c1e863353d03a30781217 Cr-Commit-Position: refs/heads/master@{#368078} ========== to ========== Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks. If the SequencedWorkerPool is running an unsequenced task, it will assign a new sequence token to it and use that for the returned SequencedTaskRunner. Committed: https://crrev.com/5d315e4b4a7cdef34b0c1e863353d03a30781217 Cr-Commit-Position: refs/heads/master@{#368078} ==========
Message was sent while issue was closed.
This change causes GCC builds to fail, there are a few errors that look like this: ../../content/browser/plugin_service_impl.cc:180:55: error: ignoring return value of 'static base::SequencedWorkerPool* content::BrowserThread::GetBlockingPool()', declared with attribute warn_unused_result [-Werror=unused-result] This is the code on that line: plugin_list_token_ = BrowserThread::GetBlockingPool()->GetSequenceToken(); ie the result of BrowserThread::GetBlockingPool() is not stored in a variable, but it is used via the named parameter idiom/method chaining. IIRC there are still some chromium-based google products using GCC, which aren't checked in the CQ.
Message was sent while issue was closed.
Followup: https://codereview.chromium.org/1567983003/
Message was sent while issue was closed.
FYI, I changed my mind about this and have a plan to change it : http://crbug.com/618043 if you still care :-). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
