Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9663075:
Implementation of SequencedTaskRunner based on SequencedWorkerPool. (Closed)
|
Created:
8 years, 9 months ago by Francois Modified:
8 years, 8 months ago CC:
chromium-reviews, brettw-cc_chromium.org, satorux1 Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionImplementation of SequencedTaskRunner based on SequencedWorkerPool.
Also includes specification tests for SequencedTaskRunner.
BUG=114330, 114327
TEST=--gtest_filter=SequencedWorkerPoolTaskRunner*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130113
Patch Set 1 #
Total comments: 35
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 10
Patch Set 4 : #
Total comments: 72
Patch Set 5 : #
Total comments: 41
Patch Set 6 : #
Total comments: 1
Patch Set 7 : #Messages
Total messages: 32 (0 generated)
This is a good starting point! Some comments. https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... File base/sequenced_task_runner_impl.cc (right): https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.cc:59: DLOG_IF(WARNING, (delay_ms != 0)) << "SequencedTaskRunnerImpl does not yet " probably should change this to NOTREACHED, analogous to SWP https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... File base/sequenced_task_runner_impl.h (right): https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This file should probably be live in threading/ side-by-side with SequencedWorkerPool. https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.h:5: #ifndef BASE_SEQUENCED_TASK_RUNNER_IMPL_H_ Conceptually, this class is essentially an inner class of SWP, although it's unwieldy to have it actually be such. I'm not a big fan of the name but I can't think of a better one just yet. https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.h:19: explicit SequencedTaskRunnerImpl(scoped_refptr<SequencedWorkerPool> pool); I think you should make the token a parameter of the constructor, just in case the client has a named one he wants to provide. I was thinking that SWP would have a method: scoped_refptr<SequencedTaskRunner> GetSequencedTaskRunner(SequenceToken token); that would return an instance of this class https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.h:24: int64 delay_ms) OVERRIDE; #include compiler_specific.h for OVERRIDE https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.h:45: class SequenceToken; No need to have another class just to avoid including sequenced_task_runner.h, especially if you make a SequenceToken a parameter https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... File base/test/sequenced_task_runner_test_template.h (right): https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:31: class SeqTaskTracker : public RefCountedThreadSafe<SeqTaskTracker> { SeqTaskTracker -> SequencedTaskTracker (abbreviations are discouraged by style guide) https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:31: class SeqTaskTracker : public RefCountedThreadSafe<SeqTaskTracker> { put this class in the base::internal namespace (you may want to take another look at task_runner_test_template.{h,cc} -- I cleaned it up a bit from an earlier version. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:36: class TaskStatus { Hmm I think this class may be overkill. I was thinking that SequencedTaskTracker would have a maps from int to vectors of ints protected by a lock, and it would provide closures that, for given integers i, j appends j to the vector for i under lock. Then the tests would simply keep track of the expected order of the integers (given the STR guarantees) in each vector and compare the actual order after all the tasks are run. Basically you have a bunch of tasks sequences and you want to make sure that all the tasks in each sequence are run in the right order. (but you don't care about interleavings between the task sequences) Comparing with TaskTracker, something like: class SequencedTaskTracker { .. // Returns a closure that runs the given task and appends |j| to the task sequence for |i|. |task| may be null. // It is guaranteed that only one task wrapped by a given tracker will be run at a time. Closure WrapTask(const Closure& task, int i, int j); std::map<int, std::vector<int> > GetTaskSequences() const; private: mutable Lock task_sequences_lock_; std::map<int, std::vector<int> > task_sequences_lock_; } I think you should be able to do everything you want with the tests below with this. What do you think? https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:73: void SetNonNestableTaskCount(const std::size_t task_count); remove const (doesn't make sense for value parameters) https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:92: scoped_refptr<SequencedTaskRunner> tr, tr -> task_runner (abbreviations are discouraged) https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:109: int ClaimNonNestableTaskStatus(const int search_from = 0); default arguments are prohibited by style guide https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:113: int ClaimNestableTaskStatus(const int search_from = 0); here too https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:121: std::ostream& operator<<(std::ostream& os, a ToString() member function is preferred to overloading operator<< https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:187: const int task_count = 20; if you change the logs in PostDelayed...Task to use NOTREACHED(), you'll have to use this->delegate_.SupportsDelayedTasks() (or whatever it's called here) https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:290: Add a TODO for a test similar to the above, but having some of the tasks call a method on the delegate that runs tasks nestedly. You'll also need to add some sort of query method for whether the implementation has such a method. The test won't be useful for STRImpl, but would be for e.g. MessageLoopProxy. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:299: #endif //#define BASE_TASK_RUNNER_TEST_TEMPLATE_H_ no '#define' in comment
sigh, i forgot to send this out last night :/ http://codereview.chromium.org/9663075/diff/1/base/test/sequenced_task_runner... File base/test/sequenced_task_runner_test_template.h (right): http://codereview.chromium.org/9663075/diff/1/base/test/sequenced_task_runner... base/test/sequenced_task_runner_test_template.h:36: class TaskStatus { On 2012/03/13 07:27:11, akalin wrote: > I think you should be able to do everything you want with the tests below with > this. What do you think? Actually a few corrections. WrapTask should *not* guarantee mutual exclusion, as that's the STR's job! It should guarantee only atomic updates to the map of task sequences. Also, since SequencedTaskTracker doesn't guarantee FIFOness, we need to track the order in which tasks start and end rather than when they run. So instead of a map of int vectors, probably want to define a struct with an int and enum { BEGIN, END } and have a map of vectors of those.
Hi Fred Thanks for the review. Your comments have been addressed. The only difference between patch set 2 and 1, besides the changes made in response to your comments, is the addition of a test (SequentialNestable) which checks that nestable tasks are started in FIFO order. Previously I only checked that they all finished. Thanks, Francois https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... File base/sequenced_task_runner_impl.cc (right): https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.cc:59: DLOG_IF(WARNING, (delay_ms != 0)) << "SequencedTaskRunnerImpl does not yet " On 2012/03/13 07:27:11, akalin wrote: > probably should change this to NOTREACHED, analogous to SWP Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... File base/sequenced_task_runner_impl.h (right): https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/03/13 07:27:11, akalin wrote: > This file should probably be live in threading/ side-by-side with > SequencedWorkerPool. Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.h:5: #ifndef BASE_SEQUENCED_TASK_RUNNER_IMPL_H_ On 2012/03/13 07:27:11, akalin wrote: > Conceptually, this class is essentially an inner class of SWP, although it's > unwieldy to have it actually be such. I'm not a big fan of the name but I can't > think of a better one just yet. Yeah, I agree, the name could use some work. I will also think about it. https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.h:19: explicit SequencedTaskRunnerImpl(scoped_refptr<SequencedWorkerPool> pool); On 2012/03/13 07:27:11, akalin wrote: > I think you should make the token a parameter of the constructor, just in case > the client has a named one he wants to provide. > > I was thinking that SWP would have a method: > > scoped_refptr<SequencedTaskRunner> GetSequencedTaskRunner(SequenceToken token); > > that would return an instance of this class Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.h:24: int64 delay_ms) OVERRIDE; On 2012/03/13 07:27:11, akalin wrote: > #include compiler_specific.h for OVERRIDE Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/sequenced_task_run... base/sequenced_task_runner_impl.h:45: class SequenceToken; On 2012/03/13 07:27:11, akalin wrote: > No need to have another class just to avoid including sequenced_task_runner.h, > especially if you make a SequenceToken a parameter Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... File base/test/sequenced_task_runner_test_template.h (right): https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:31: class SeqTaskTracker : public RefCountedThreadSafe<SeqTaskTracker> { On 2012/03/13 07:27:11, akalin wrote: > SeqTaskTracker -> SequencedTaskTracker (abbreviations are discouraged by style > guide) Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:31: class SeqTaskTracker : public RefCountedThreadSafe<SeqTaskTracker> { On 2012/03/13 07:27:11, akalin wrote: > SeqTaskTracker -> SequencedTaskTracker (abbreviations are discouraged by style > guide) Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:36: class TaskStatus { On 2012/03/13 20:00:10, akalin wrote: > On 2012/03/13 07:27:11, akalin wrote: > > I think you should be able to do everything you want with the tests below with > > this. What do you think? > > Actually a few corrections. WrapTask should *not* guarantee mutual exclusion, > as that's the STR's job! It should guarantee only atomic updates to the map of > task sequences. > > Also, since SequencedTaskTracker doesn't guarantee FIFOness, we need to track > the order in which tasks start and end rather than when they run. So instead of > a map of int vectors, probably want to define a struct with an int and enum { > BEGIN, END } and have a map of vectors of those. I like your model because it mirrors the task hierarchy well, but I don't think it's going to work for all my tests. E.g. I have one (NonNestablePostFromNonNestableTask) which posts non-nestable tasks from non-nestable ones. For this one it's important to verify that all tasks, whether root or sub, were run in the order they were posted and without execution overlap. The order in which they're posted is unknown because the posts are done from different threads. With your model I wouldn't be able to verify that if, for example, the posting order was {T0, ST0_0, T1, ST0_1}, that T1 started only after ST0_0 had finished, or that ST0_1 started after T1 had finished. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:73: void SetNonNestableTaskCount(const std::size_t task_count); On 2012/03/13 07:27:11, akalin wrote: > remove const (doesn't make sense for value parameters) Done. But it does make sense to me :) If something isn't supposed to change (the function's local copy of the argument, in this case), mark it const. I don't think it's much different from declaring a regular function-local variable const. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:92: scoped_refptr<SequencedTaskRunner> tr, On 2012/03/13 07:27:11, akalin wrote: > tr -> task_runner (abbreviations are discouraged) Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:109: int ClaimNonNestableTaskStatus(const int search_from = 0); On 2012/03/13 07:27:11, akalin wrote: > default arguments are prohibited by style guide Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:113: int ClaimNestableTaskStatus(const int search_from = 0); On 2012/03/13 07:27:11, akalin wrote: > here too Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:121: std::ostream& operator<<(std::ostream& os, On 2012/03/13 07:27:11, akalin wrote: > a ToString() member function is preferred to overloading operator<< OK, but I added this to customise the output of EXPECT_EQ when it evaluates to false. Without this it prints byte values. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:187: const int task_count = 20; On 2012/03/13 07:27:11, akalin wrote: > if you change the logs in PostDelayed...Task to use NOTREACHED(), you'll have to > use this->delegate_.SupportsDelayedTasks() (or whatever it's called here) Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:290: On 2012/03/13 07:27:11, akalin wrote: > Add a TODO for a test similar to the above, but having some of the tasks call a > method on the delegate that runs tasks nestedly. You'll also need to add some > sort of query method for whether the implementation has such a method. The test > won't be useful for STRImpl, but would be for e.g. MessageLoopProxy. Done. https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas... base/test/sequenced_task_runner_test_template.h:299: #endif //#define BASE_TASK_RUNNER_TEST_TEMPLATE_H_ On 2012/03/13 07:27:11, akalin wrote: > no '#define' in comment Done.
On 2012/03/14 15:43:31, Francois wrote:
> I like your model because it mirrors the task hierarchy well, but I don't
think
> it's going to work for all my tests. E.g. I have one
> (NonNestablePostFromNonNestableTask) which posts non-nestable tasks from
> non-nestable ones. For this one it's important to verify that all tasks,
whether
> root or sub, were run in the order they were posted and without execution
> overlap. The order in which they're posted is unknown because the posts are
done
> from different threads. With your model I wouldn't be able to verify that if,
> for example, the posting order was {T0, ST0_0, T1, ST0_1}, that T1 started
only
> after ST0_0 had finished, or that ST0_1 started after T1 had finished.
I see. Did you see my follow-up comment, where I realized we'd need to keep
track of both start and end time? If you also want to keep track of post time,
you can maybe define a SequencedTaskRunner wrapper that also keeps track of the
post time for a task in a TaskSequence (or maybe make SequencedTaskRunner
directly implement SequencedTaskRunner also). Then at the very end, you can get
a TaskSequence (which looks like T0-post, T0-start, T1-post, T0-end, T1-start
...), scan it to get the post order, then scan it again to make sure start/ends
are in the same order and don't overlap.
I think that's a more understandable way to structure the tests.
>
https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas...
> base/test/sequenced_task_runner_test_template.h:73: void
> SetNonNestableTaskCount(const std::size_t task_count);
> On 2012/03/13 07:27:11, akalin wrote:
> > remove const (doesn't make sense for value parameters)
>
> Done. But it does make sense to me :) If something isn't supposed to change
(the
> function's local copy of the argument, in this case), mark it const. I don't
> think it's much different from declaring a regular function-local variable
> const.
The problem is that it's already being copied, so you can't guarantee that the
object doesn't change (whatever that means) since the object can just be copied.
If you want to "really" guarantee immutability, const& is the way to go,
although for integral types like these, the caller shouldn't care (or know)
whether the function reuses the argument for a computation or something.
>
https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas...
> base/test/sequenced_task_runner_test_template.h:92:
> scoped_refptr<SequencedTaskRunner> tr,
> On 2012/03/13 07:27:11, akalin wrote:
> > tr -> task_runner (abbreviations are discouraged)
>
> Done.
>
>
https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas...
> base/test/sequenced_task_runner_test_template.h:109: int
> ClaimNonNestableTaskStatus(const int search_from = 0);
> On 2012/03/13 07:27:11, akalin wrote:
> > default arguments are prohibited by style guide
>
> Done.
>
>
https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas...
> base/test/sequenced_task_runner_test_template.h:113: int
> ClaimNestableTaskStatus(const int search_from = 0);
> On 2012/03/13 07:27:11, akalin wrote:
> > here too
>
> Done.
>
>
https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas...
> base/test/sequenced_task_runner_test_template.h:121: std::ostream&
> operator<<(std::ostream& os,
> On 2012/03/13 07:27:11, akalin wrote:
> > a ToString() member function is preferred to overloading operator<<
>
> OK, but I added this to customise the output of EXPECT_EQ when it evaluates to
> false. Without this it prints byte values.
Ah, if you want to do that, you can instead have a PrintTo function, as
described in:
http://code.google.com/p/googletest/wiki/AdvancedGuide#Teaching_Google_Test_H...
>
>
https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas...
> base/test/sequenced_task_runner_test_template.h:187: const int task_count =
20;
> On 2012/03/13 07:27:11, akalin wrote:
> > if you change the logs in PostDelayed...Task to use NOTREACHED(), you'll
have
> to
> > use this->delegate_.SupportsDelayedTasks() (or whatever it's called here)
>
> Done.
>
>
https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas...
> base/test/sequenced_task_runner_test_template.h:290:
> On 2012/03/13 07:27:11, akalin wrote:
> > Add a TODO for a test similar to the above, but having some of the tasks
call
> a
> > method on the delegate that runs tasks nestedly. You'll also need to add
some
> > sort of query method for whether the implementation has such a method. The
> test
> > won't be useful for STRImpl, but would be for e.g. MessageLoopProxy.
>
> Done.
>
>
https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas...
> base/test/sequenced_task_runner_test_template.h:299: #endif //#define
> BASE_TASK_RUNNER_TEST_TEMPLATE_H_
> On 2012/03/13 07:27:11, akalin wrote:
> > no '#define' in comment
>
> Done.
More comments http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_tas... File base/threading/sequenced_task_runner_impl.h (right): http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_tas... base/threading/sequenced_task_runner_impl.h:20: public base::SupportsWeakPtr<SequencedTaskRunnerImpl> { It doesn't make sense for a ref-counted-thread-safe object to support weak pointers, as weak pointers are non-thread-safe. http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_tas... base/threading/sequenced_task_runner_impl.h:24: SequencedTaskRunnerImpl(scoped_refptr<SequencedWorkerPool> pool, just one constructor, for pool + token http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_wor... File base/threading/sequenced_worker_pool.cc (right): http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_wor... base/threading/sequenced_worker_pool.cc:86: scoped_refptr<SequencedTaskRunner> GetSequencedTaskRunner( Overloads are against style guide -- I think there should only be one function that takes a SequenceToken; users can either query for a new one or map from a token_name if they need to. http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_wor... base/threading/sequenced_worker_pool.cc:176: WeakPtr<SequencedTaskRunner> sequenced_task_runner_; no need for this member variable. Creating a new STRImpl is cheap enough that there's no need to reuse one. http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_wor... base/threading/sequenced_worker_pool.cc:336: AutoLock lock(lock_); per the comments above, shouldn't need a lock for this
On 2012/03/15 19:17:24, akalin wrote:
> On 2012/03/14 15:43:31, Francois wrote:
> > I like your model because it mirrors the task hierarchy well, but I don't
> think
> > it's going to work for all my tests. E.g. I have one
> > (NonNestablePostFromNonNestableTask) which posts non-nestable tasks from
> > non-nestable ones. For this one it's important to verify that all tasks,
> whether
> > root or sub, were run in the order they were posted and without execution
> > overlap. The order in which they're posted is unknown because the posts are
> done
> > from different threads. With your model I wouldn't be able to verify that
if,
> > for example, the posting order was {T0, ST0_0, T1, ST0_1}, that T1 started
> only
> > after ST0_0 had finished, or that ST0_1 started after T1 had finished.
>
> I see. Did you see my follow-up comment, where I realized we'd need to keep
> track of both start and end time?
I did see your follow-up comments, but my tests were already checking the
relative start and end order, in case you didn't see that.
> If you also want to keep track of post time,
> you can maybe define a SequencedTaskRunner wrapper that also keeps track of
the
> post time for a task in a TaskSequence (or maybe make SequencedTaskRunner
> directly implement SequencedTaskRunner also). Then at the very end, you can
get
> a TaskSequence (which looks like T0-post, T0-start, T1-post, T0-end, T1-start
> ...), scan it to get the post order, then scan it again to make sure
start/ends
> are in the same order and don't overlap.
>
> I think that's a more understandable way to structure the tests.
I went with a variation on your solution. I keep each event type (post, start,
end) in a separate vector because it decreases contention and it makes the
assertion logic simpler (e.g. a simple logical equality comparison in most
cases).
I have noticed a general problem, though. It's not possible to verify that
nestable tasks start in FIFO order. There will always be a race between the
start of the task function and its first C++ statement (the one which records
that it has started). E.g. if the post order was {T0, T1}, it is possible that
1) T0 could be scheduled and interrupted before it gets a chance to record that
it has started, causing T1's start to be recorded before T0's, resulting in a
failure for a test that should've passed.
2) T1 could've been scheduled and interrupted before it got a chance to record
that it has started, causing T0's start to be recorded before T1's, resulting in
a pass for a test that should've failed.
This can be seen by running the test
SequencedTaskRunnerImpl/SequencedTaskRunnerTest/0.FAILS_SequentialNestable. It
fails quite consistently, and one can tell from the output that one or more of
the tasks's starts are recorded in the wrong order.
>
> >
>
https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas...
> > base/test/sequenced_task_runner_test_template.h:73: void
> > SetNonNestableTaskCount(const std::size_t task_count);
> > On 2012/03/13 07:27:11, akalin wrote:
> > > remove const (doesn't make sense for value parameters)
> >
> > Done. But it does make sense to me :) If something isn't supposed to change
> (the
> > function's local copy of the argument, in this case), mark it const. I don't
> > think it's much different from declaring a regular function-local variable
> > const.
>
> The problem is that it's already being copied, so you can't guarantee that the
> object doesn't change (whatever that means) since the object can just be
copied.
> If you want to "really" guarantee immutability, const& is the way to go,
> although for integral types like these, the caller shouldn't care (or know)
> whether the function reuses the argument for a computation or something.
>
I am not talking about changing the value of the variable at the call site, I am
talking about ensuring that the function-local copy cannot change value inside
the function. E.g.
void f(const int i) {
i = 77; // compilation error
}
> >
>
https://chromiumcodereview.appspot.com/9663075/diff/1/base/test/sequenced_tas...
> > base/test/sequenced_task_runner_test_template.h:121: std::ostream&
> > operator<<(std::ostream& os,
> > On 2012/03/13 07:27:11, akalin wrote:
> > > a ToString() member function is preferred to overloading operator<<
> >
> > OK, but I added this to customise the output of EXPECT_EQ when it evaluates
to
> > false. Without this it prints byte values.
>
> Ah, if you want to do that, you can instead have a PrintTo function, as
> described in:
>
>
http://code.google.com/p/googletest/wiki/AdvancedGuide#Teaching_Google_Test_H...
>
Thanks for making me aware of this, but with my latest changes I no longer
require a custom output function.
Hi Fred, I have addressed your comments. For some I would like some more feedback before making changes. Thanks! http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_tas... File base/threading/sequenced_task_runner_impl.h (right): http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_tas... base/threading/sequenced_task_runner_impl.h:24: SequencedTaskRunnerImpl(scoped_refptr<SequencedWorkerPool> pool, On 2012/03/15 19:17:35, akalin wrote: > just one constructor, for pool + token Done. http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_wor... File base/threading/sequenced_worker_pool.cc (right): http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_wor... base/threading/sequenced_worker_pool.cc:86: scoped_refptr<SequencedTaskRunner> GetSequencedTaskRunner( On 2012/03/15 19:17:35, akalin wrote: > Overloads are against style guide -- I think there should only be one function > that takes a SequenceToken; users can either query for a new one or map from a > token_name if they need to. Done. http://codereview.chromium.org/9663075/diff/7001/base/threading/sequenced_wor... base/threading/sequenced_worker_pool.cc:176: WeakPtr<SequencedTaskRunner> sequenced_task_runner_; On 2012/03/15 19:17:35, akalin wrote: > no need for this member variable. Creating a new STRImpl is cheap enough that > there's no need to reuse one. Do you mean that it should do "return scoped_refptr<STRImpl>(new STRImpl)"? If so, I did consider it, but I reckoned that it would be pointless to return a scoped_refptr to a new instance every time. I thought it would be better to return a scoped_ptr (with a new instance) instead, but I got the impression that it was quite important for STR to be ref-counted. This is why I went with a persistent instance inside SWP, which would keep a weak pointer to it. But I didn't realise that weak_ptrs are not thread-safe. So it will have to be one of the other two options, but which one? http://codereview.chromium.org/9663075/diff/14001/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.cc (right): http://codereview.chromium.org/9663075/diff/14001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.cc:78: task_runner->PostNonNestableTask(FROM_HERE, task); The loop body is messy, but I want to hear your comments before I change this. http://codereview.chromium.org/9663075/diff/14001/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.h (right): http://codereview.chromium.org/9663075/diff/14001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:230: task_runner->PostNonNestableTask(FROM_HERE, task); The loop body is messy, but I want to hear your comments before I change this. http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... File base/threading/sequenced_task_runner_impl.cc (right): http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:47: "support non-zero delays; ignoring delay of " << delay_ms << " ms"; I shouldn't have changed this back; I will fix it again in the next patch set. http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:59: "ignoring delay of " << delay.InMillisecondsRoundedUp(); I shouldn't have changed this back; I will fix it again in the next patch set.
On 2012/03/18 16:13:58, Francois wrote:
> I went with a variation on your solution. I keep each event type (post, start,
> end) in a separate vector because it decreases contention and it makes the
> assertion logic simpler (e.g. a simple logical equality comparison in most
> cases).
Please stick with my solution. Your solution loses information needlessly; you
can't verify that e.g. a given task ends after it starts and starts after it's
posted. There's no need to worry about contention; this is test code.
> I have noticed a general problem, though. It's not possible to verify that
> nestable tasks start in FIFO order. There will always be a race between the
> start of the task function and its first C++ statement (the one which records
> that it has started). E.g. if the post order was {T0, T1}, it is possible that
>
> 1) T0 could be scheduled and interrupted before it gets a chance to record
that
> it has started, causing T1's start to be recorded before T0's, resulting in a
> failure for a test that should've passed.
>
> 2) T1 could've been scheduled and interrupted before it got a chance to record
> that it has started, causing T0's start to be recorded before T1's, resulting
in
> a pass for a test that should've failed.
>
> This can be seen by running the test
> SequencedTaskRunnerImpl/SequencedTaskRunnerTest/0.FAILS_SequentialNestable. It
> fails quite consistently, and one can tell from the output that one or more of
> the tasks's starts are recorded in the wrong order.
It seems like you're still misunderstanding nestable tasks. With sequenced
worker pools, there's _no difference_ between nestable and non-nestable tasks,
since there's no way to pump the loop from within a task running in the worker
pool. So there should be _no difference_ between your SequentialNestable test
and SequentialNonNestable test.
Indeed, it reveals a bug in your implementation; SWPImpl::PostDelayedTask should
*not* forward to pool_->PostDelayedTask, but to pool_->PostSequencedWorkerTask,
like SWPImpl::PostNonNestableDelayedTask. (Remember, there's no difference
between nestable/non-nestable with SWP.
> I am not talking about changing the value of the variable at the call site, I
am
> talking about ensuring that the function-local copy cannot change value inside
> the function. E.g.
>
> void f(const int i) {
> i = 77; // compilation error
> }
I know. But why should the caller care about that? Anyway, it's not important.
On 2012/03/18 16:28:29, Francois wrote: > Do you mean that it should do "return scoped_refptr<STRImpl>(new STRImpl)"? If > so, I did consider it, but I reckoned that it would be pointless to return a > scoped_refptr to a new instance every time. I thought it would be better to > return a scoped_ptr (with a new instance) instead, but I got the impression that > it was quite important for STR to be ref-counted. This is why I went with a > persistent instance inside SWP, which would keep a weak pointer to it. But I > didn't realise that weak_ptrs are not thread-safe. So it will have to be one of > the other two options, but which one? But you _have to_ return a new object each time, because the token may be different. Remember, a SequencedWorkerPool is a _general_ worker pool that happens to expose methods that serializes methods for a given token. A SWPImpl is a (pool, token) pair, which behaves like a SequencedTaskRunner.
http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... File base/threading/sequenced_task_runner_impl.cc (right): http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:26: return pool_->PostDelayedTask(from_here, task, delay_ms); PostDelayedTask -> PostSequencedWorkerTask http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:33: return pool_->PostDelayedTask(from_here, task, delay); PostDelayedTask -> PostSequencedWorkerTask
On 2012/03/18 16:13:58, Francois wrote:
> I have noticed a general problem, though. It's not possible to verify that
> nestable tasks start in FIFO order. There will always be a race between the
> start of the task function and its first C++ statement (the one which records
> that it has started). E.g. if the post order was {T0, T1}, it is possible that
>
> 1) T0 could be scheduled and interrupted before it gets a chance to record
that
> it has started, causing T1's start to be recorded before T0's, resulting in a
> failure for a test that should've passed.
>
> 2) T1 could've been scheduled and interrupted before it got a chance to record
> that it has started, causing T0's start to be recorded before T1's, resulting
in
> a pass for a test that should've failed.
>
> This can be seen by running the test
> SequencedTaskRunnerImpl/SequencedTaskRunnerTest/0.FAILS_SequentialNestable. It
> fails quite consistently, and one can tell from the output that one or more of
> the tasks's starts are recorded in the wrong order.
To clarify even more, The situations you describe should only happen with
TaskRunner. SequencedTaskRunner guarantees that T0 will finish before T1 starts
(unless T0 pumps the message loop).
On 2012/03/19 09:28:17, akalin wrote: > On 2012/03/18 16:13:58, Francois wrote: > > I went with a variation on your solution. I keep each event type (post, start, > > end) in a separate vector because it decreases contention and it makes the > > assertion logic simpler (e.g. a simple logical equality comparison in most > > cases). > > Please stick with my solution. Your solution loses information needlessly; you > can't verify that e.g. a given task ends after it starts and starts after it's > posted. There's no need to worry about contention; this is test code. I've now gone with what I understand to be your solution. I hope I got it right. But how can an implementation cause the order of posts, starts, and ends for a task to occur in the wrong order? The only ways I could think of were if the implementation ran the task (overlapped, perhaps) more times than it was posted, or if the test code messed up and assigned same i to different tasks or did something similar wrong. > It seems like you're still misunderstanding nestable tasks. With sequenced > worker pools, there's _no difference_ between nestable and non-nestable tasks, > since there's no way to pump the loop from within a task running in the worker > pool. So there should be _no difference_ between your SequentialNestable test > and SequentialNonNestable test. > > > Indeed, it reveals a bug in your implementation; SWPImpl::PostDelayedTask should > *not* forward to pool_->PostDelayedTask, but to pool_->PostSequencedWorkerTask, > like SWPImpl::PostNonNestableDelayedTask. (Remember, there's no difference > between nestable/non-nestable with SWP. You're right, I was still misunderstanding. I've changed the implementation and tests accordingly.
On 2012/03/19 09:31:32, akalin wrote: > On 2012/03/18 16:28:29, Francois wrote: > > Do you mean that it should do "return scoped_refptr<STRImpl>(new STRImpl)"? If > > so, I did consider it, but I reckoned that it would be pointless to return a > > scoped_refptr to a new instance every time. I thought it would be better to > > return a scoped_ptr (with a new instance) instead, but I got the impression > that > > it was quite important for STR to be ref-counted. This is why I went with a > > persistent instance inside SWP, which would keep a weak pointer to it. But I > > didn't realise that weak_ptrs are not thread-safe. So it will have to be one > of > > the other two options, but which one? > > But you _have to_ return a new object each time, because the token may be > different. Remember, a SequencedWorkerPool is a _general_ worker pool that > happens to expose methods that serializes methods for a given token. A SWPImpl > is a (pool, token) pair, which behaves like a SequencedTaskRunner. Yes, I see that now. I've made the changes.
Hi Fred, I've addressed your comments. Please see patch set 4 for the latest changes. Thanks! http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... File base/threading/sequenced_task_runner_impl.cc (right): http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:26: return pool_->PostDelayedTask(from_here, task, delay_ms); On 2012/03/19 09:34:21, akalin wrote: > PostDelayedTask -> PostSequencedWorkerTask Done. http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:33: return pool_->PostDelayedTask(from_here, task, delay); On 2012/03/19 09:34:21, akalin wrote: > PostDelayedTask -> PostSequencedWorkerTask Done. http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:47: "support non-zero delays; ignoring delay of " << delay_ms << " ms"; On 2012/03/18 16:28:29, Francois wrote: > I shouldn't have changed this back; I will fix it again in the next patch set. Done. http://codereview.chromium.org/9663075/diff/14001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:59: "ignoring delay of " << delay.InMillisecondsRoundedUp(); On 2012/03/18 16:28:29, Francois wrote: > I shouldn't have changed this back; I will fix it again in the next patch set. Done.
Okay, this is looking better. more omments. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.cc (right): http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.cc:43: base::PlatformThread::YieldCurrentThread(); remove this line -- it shouldn't be needed and its use is subtle http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.h (right): http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:6: // pass in order to be conformant. Here's how you use it to test your Remove 2nd sentence, merge paragraph below with this one http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:17: remove extra newline http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:35: struct TaskPhase { I think TaskEvent is a better name http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:38: bool operator==(const TaskPhase& phase) { prefer bool Equals(const TaskPhase& phase) const { } http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:41: int i_; typically public member variables don't have an appended _ http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:47: int GetNextPostOrdinal(); shouldn't need this function (see comments below) http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:49: void TaskPosted(int i); these functions shouldn't be exposed. Instead, you should expose something like: void PostWrappedTask(const scoped_refptr<TaskRunner>&, const Closure& task, int i); which should call TaskPosted and post a wrapper task that calls TaskStarted/TaskEnded, etc. (also, it should support a null task) Then you can just define: void DoNothing() { } void SleepForOneSecond() { ...Sleep(...); } etc. and post those http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:69: public: these shouldn't be public http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:71: mutable Lock phase_lock_; add a comment saying that phase_lock_ protects phases_ (even though it's obvious) http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:111: if (i == 0) { with the comments above, you can move the i=0 case out of the loop, e.g. this->task_tracker_->PostNonNestableWrappedTask(FROM_HERE, Bind(&SleepForOneSecond), 0); for (int i = 0; i < task_count; ++i) { this->task_tracker_->PostNonNestableWrappedTask(FROM_HERE, Closure(), i); } http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:233: template <typename TaskRunnerTestDelegate> the code below should probably go before the unit tests themselves http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:235: SequencedTaskRunnerTest<TaskRunnerTestDelegate>::TaskPhasesInOrder( this function does a bit too much -- it's complicated. I think it would be better to split out the tests. Here's what I suggest: - Define the function: std::vector<int> GetEventTypeOrder(const std::vector<TaskEvent>& events, TaskEvent::Type type); which basically filters by type and returns the i's. Then you can check that GetEventTypeOrder(events, POST) == GetEventTypeOrder(events, START) and ...START) == ...END) (which holds if there are no nested tasks, but all of the current tests assume that anyway, and we can handle nestable tasks later). Then you can similarly define a function that checks that for all i, POST comes before START comes before END, and then another one that makes sure the only thing between START and END (for a given i) are POST events (for j != i, no nestable tasks). Then I think you can wrap all those checks in some function CheckNonNestableInvariants(...) and call that from the tests. These functions should be free functions (i.e., not part of a class). http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... File base/threading/sequenced_task_runner_impl.cc (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:8: #include "base/threading/sequenced_worker_pool.h" no need for this #include (since you include from header file) http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:46: DCHECK_EQ(delay_ms, 0) << "SequencedTaskRunnerImpl does not yet support " indent before <<, like you do below http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:54: base::TimeDelta delay) { remove base:: http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... File base/threading/sequenced_task_runner_impl.h (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:5: #ifndef BASE_THREADING_SEQUENCED_TASK_RUNNER_IMPL_H_ After thinking about it, I think the best name for this class would be: SequencedWorkerPoolTaskRunner The only danger is that a user may think this implements just TaskRunner when he needs a SequencedTaskRunner. But if the user knows that SequencedWorkerPool already implements TaskRunner or that SWPTaskRunner takes a SequenceToken, he should be ok. What do you think? http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:10: #include "base/memory/scoped_ptr.h" remove scoped_ptr include, add ref_counted.h include http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:17: class BASE_EXPORT SequencedTaskRunnerImpl : public base::SequencedTaskRunner { remove base:: http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:18: public: indent one space http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:19: SequencedTaskRunnerImpl(scoped_refptr<SequencedWorkerPool> pool, use const scoped_refptr<...>& http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:39: base::TimeDelta delay) OVERRIDE; remove base:: http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:41: private: indent one space http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:42: ~SequencedTaskRunnerImpl(); destructor needs to be virtual http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:44: scoped_refptr<SequencedWorkerPool> pool_; make this const http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:46: SequencedWorkerPool::SequenceToken token_; this, too http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:48: DISALLOW_COPY_AND_ASSIGN(SequencedTaskRunnerImpl); need #include "base/basictypes.h" for this http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:732: scoped_refptr<SequencedWorkerPool>(this), token); I think you can just pass 'this' and it'll implicitly construct the scoped_refptr http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.h (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.h:157: // Returns a SequencedTaskRunner wrapper for this SequencedWorkerPool and Perhaps clearer: // Returns a SequencedTaskRunner wrapper that posts to this SequencedWorkerPool with the given sequence token. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:24: SequencedWorkerPoolOwner::SequencedWorkerPoolOwner( move this in the corresponding file base/test/sequenced_worker_pool_owner.cc http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.h (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:6: #define BASE_THREADING_SEQUENCED_WORKER_POOL_UNITTEST_H_ i think it's better to put this in a new file sequenced_worker_pool_owner.h in base/test http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:18: // Wrapper around SequencedWorkerPool that blocks destruction until SequencedWorkerPool that -> SequencedWorkerPool for testing that http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:32: // Don't change the return pool's testing observer. return -> returned http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:42: virtual void OnHasWork() OVERRIDE; need compiler_specific.h include for this http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:47: scoped_refptr<SequencedWorkerPool> pool_; need base/memory/ref_counted.h include for this http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:53: DISALLOW_COPY_AND_ASSIGN(SequencedWorkerPoolOwner); need #include "base/basictypes.h" for this
Okay, this is looking better. more comments.
subscribed by adding me in the cc list
Hi Fred, comments have been addressed. Thanks! http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.cc (right): http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.cc:43: base::PlatformThread::YieldCurrentThread(); On 2012/03/20 22:16:08, akalin wrote: > remove this line -- it shouldn't be needed and its use is subtle Done. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.h (right): http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:6: // pass in order to be conformant. Here's how you use it to test your On 2012/03/20 22:16:08, akalin wrote: > Remove 2nd sentence, merge paragraph below with this one Done. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:17: On 2012/03/20 22:16:08, akalin wrote: > remove extra newline Done. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:35: struct TaskPhase { On 2012/03/20 22:16:08, akalin wrote: > I think TaskEvent is a better name Done. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:38: bool operator==(const TaskPhase& phase) { On 2012/03/20 22:16:08, akalin wrote: > prefer > > bool Equals(const TaskPhase& phase) const { > } Will do next time, but it isn't required anymore, so I've removed it. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:41: int i_; On 2012/03/20 22:16:08, akalin wrote: > typically public member variables don't have an appended _ Done. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:47: int GetNextPostOrdinal(); On 2012/03/20 22:16:08, akalin wrote: > shouldn't need this function (see comments below) Done. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:49: void TaskPosted(int i); On 2012/03/20 22:16:08, akalin wrote: > these functions shouldn't be exposed. Instead, you should expose something > like: > > void PostWrappedTask(const scoped_refptr<TaskRunner>&, const Closure& task, int > i); > > which should call TaskPosted and post a wrapper task that calls > TaskStarted/TaskEnded, etc. (also, it should support a null task) > > Then you can just define: > > void DoNothing() { > } > > void SleepForOneSecond() { > ...Sleep(...); > } > > etc. > > and post those Done. And DoNothing isn't required if null tasks are supported, right? http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:69: public: On 2012/03/20 22:16:08, akalin wrote: > these shouldn't be public Done. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:71: mutable Lock phase_lock_; On 2012/03/20 22:16:08, akalin wrote: > add a comment saying that phase_lock_ protects phases_ (even though it's > obvious) Done. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:111: if (i == 0) { On 2012/03/20 22:16:08, akalin wrote: > with the comments above, you can move the i=0 case out of the loop, e.g. > > this->task_tracker_->PostNonNestableWrappedTask(FROM_HERE, > Bind(&SleepForOneSecond), 0); > for (int i = 0; i < task_count; ++i) { > this->task_tracker_->PostNonNestableWrappedTask(FROM_HERE, Closure(), i); > } Done. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:233: template <typename TaskRunnerTestDelegate> On 2012/03/20 22:16:08, akalin wrote: > the code below should probably go before the unit tests themselves Done. http://codereview.chromium.org/9663075/diff/21001/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:235: SequencedTaskRunnerTest<TaskRunnerTestDelegate>::TaskPhasesInOrder( On 2012/03/20 22:16:08, akalin wrote: > this function does a bit too much -- it's complicated. I think it would be > better to split out the tests. Here's what I suggest: > > - Define the function: > > std::vector<int> GetEventTypeOrder(const std::vector<TaskEvent>& events, > TaskEvent::Type type); > > which basically filters by type and returns the i's. > > Then you can check that GetEventTypeOrder(events, POST) == > GetEventTypeOrder(events, START) and ...START) == ...END) (which holds if there > are no nested tasks, but all of the current tests assume that anyway, and we can > handle nestable tasks later). > > Then you can similarly define a function that checks that for all i, POST comes > before START comes before END, and then another one that makes sure the only > thing between START and END (for a given i) are POST events (for j != i, no > nestable tasks). Then I think you can wrap all those checks in some function > CheckNonNestableInvariants(...) and call that from the tests. > > These functions should be free functions (i.e., not part of a class). Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... File base/threading/sequenced_task_runner_impl.cc (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:8: #include "base/threading/sequenced_worker_pool.h" On 2012/03/20 22:16:08, akalin wrote: > no need for this #include (since you include from header file) Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:46: DCHECK_EQ(delay_ms, 0) << "SequencedTaskRunnerImpl does not yet support " On 2012/03/20 22:16:08, akalin wrote: > indent before <<, like you do below Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.cc:54: base::TimeDelta delay) { On 2012/03/20 22:16:08, akalin wrote: > remove base:: Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... File base/threading/sequenced_task_runner_impl.h (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:5: #ifndef BASE_THREADING_SEQUENCED_TASK_RUNNER_IMPL_H_ On 2012/03/20 22:16:08, akalin wrote: > After thinking about it, I think the best name for this class would be: > > SequencedWorkerPoolTaskRunner > > The only danger is that a user may think this implements just TaskRunner when he > needs a SequencedTaskRunner. But if the user knows that SequencedWorkerPool > already implements TaskRunner or that SWPTaskRunner takes a SequenceToken, he > should be ok. > > What do you think? Done. I haven't managed to think of anything better. I agree that users of this class should be familiar with SWP, so it should be OK. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:10: #include "base/memory/scoped_ptr.h" On 2012/03/20 22:16:08, akalin wrote: > remove scoped_ptr include, add ref_counted.h include Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:17: class BASE_EXPORT SequencedTaskRunnerImpl : public base::SequencedTaskRunner { On 2012/03/20 22:16:08, akalin wrote: > remove base:: Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:18: public: On 2012/03/20 22:16:08, akalin wrote: > indent one space Done. Looks like the presence of BASE_EXPORT causes Emacs (using google-c-style.el) to not indent labels. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:19: SequencedTaskRunnerImpl(scoped_refptr<SequencedWorkerPool> pool, On 2012/03/20 22:16:08, akalin wrote: > use const scoped_refptr<...>& Done. Is the goal to eliminate the extra ref++ and ref-- of the temporary? http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:39: base::TimeDelta delay) OVERRIDE; On 2012/03/20 22:16:08, akalin wrote: > remove base:: Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:41: private: On 2012/03/20 22:16:08, akalin wrote: > indent one space Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:42: ~SequencedTaskRunnerImpl(); On 2012/03/20 22:16:08, akalin wrote: > destructor needs to be virtual Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:44: scoped_refptr<SequencedWorkerPool> pool_; On 2012/03/20 22:16:08, akalin wrote: > make this const Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:46: SequencedWorkerPool::SequenceToken token_; On 2012/03/20 22:16:08, akalin wrote: > this, too Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_ta... base/threading/sequenced_task_runner_impl.h:48: DISALLOW_COPY_AND_ASSIGN(SequencedTaskRunnerImpl); On 2012/03/20 22:16:08, akalin wrote: > need #include "base/basictypes.h" for this Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:732: scoped_refptr<SequencedWorkerPool>(this), token); On 2012/03/20 22:16:08, akalin wrote: > I think you can just pass 'this' and it'll implicitly construct the > scoped_refptr Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.h (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.h:157: // Returns a SequencedTaskRunner wrapper for this SequencedWorkerPool and On 2012/03/20 22:16:08, akalin wrote: > Perhaps clearer: > > // Returns a SequencedTaskRunner wrapper that posts to this SequencedWorkerPool > with the given sequence token. Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.cc (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.cc:24: SequencedWorkerPoolOwner::SequencedWorkerPoolOwner( On 2012/03/20 22:16:08, akalin wrote: > move this in the corresponding file base/test/sequenced_worker_pool_owner.cc Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_unittest.h (right): http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:6: #define BASE_THREADING_SEQUENCED_WORKER_POOL_UNITTEST_H_ On 2012/03/20 22:16:08, akalin wrote: > i think it's better to put this in a new file sequenced_worker_pool_owner.h in > base/test Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:18: // Wrapper around SequencedWorkerPool that blocks destruction until On 2012/03/20 22:16:08, akalin wrote: > SequencedWorkerPool that -> SequencedWorkerPool for testing that Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:32: // Don't change the return pool's testing observer. On 2012/03/20 22:16:08, akalin wrote: > return -> returned Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:42: virtual void OnHasWork() OVERRIDE; On 2012/03/20 22:16:08, akalin wrote: > need compiler_specific.h include for this Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:47: scoped_refptr<SequencedWorkerPool> pool_; On 2012/03/20 22:16:08, akalin wrote: > need base/memory/ref_counted.h include for this Done. http://codereview.chromium.org/9663075/diff/21001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_unittest.h:53: DISALLOW_COPY_AND_ASSIGN(SequencedWorkerPoolOwner); On 2012/03/20 22:16:08, akalin wrote: > need #include "base/basictypes.h" for this Done. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.cc (right): http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.cc:7: #include <ostream> For SequencedTaskTracker::TaskEvent::PrintTo, which takes a std::ostream argument.
LGTM after comments below! http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.cc (right): http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.cc:9: #include "base/tracked_objects.h" looks like this should be base/location.h (this changed recently) http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.cc:15: SequencedTaskTracker::TaskEvent::TaskEvent(int i_in, Type type_in) don't use _in, just name them the same; C++'s variable lookup rules are such that ": i(i), type(type)" works correctly. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.cc:149: for (int i = 0; i < task_count; ++i) { This takes n^2 time, which isn't great but probably not worth optimizing. But should still add a comment about it. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.h (right): http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:33: struct TaskEvent { just pull out this struct, since it's already in an internal namespace (and you're typedefing it anyway) http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:35: TaskEvent(int i_in, Type type_in); don't use _in, just name them the same http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:35: TaskEvent(int i_in, Type type_in); define a default constructor and initialize both variables to some default http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:43: void PostWrappedNonNestableTask( the index (int i) should be a member of these functions, as otherwise the caller would have to assume they're sequential http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:83: // Protects events_ and next_post_i_. events_ only http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:91: int next_post_i_; remove the need for this, since the index should be a member of the post functions http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:138: task_runner, Bind(&internal::SleepForOneSecond)); with the change above to pass i to PostWrappedNonNestableTask, keep around an expected_post_order vector<int> and push the i's onto that. Then assert that it's the same as the actual post order. (And do something similar for other tests.) http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:219: children_per_parent); probably would have to pass in a start/end index for the child tasks, too http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_task_runner.cc (right): http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.cc:26: return pool_->PostSequencedWorkerTask(token_, from_here, task); you want a DCHECK_EQ(delay_ms, 0) here, too, like below http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.cc:33: return pool_->PostSequencedWorkerTask(token_, from_here, task); here, too. In fact, you may just want to have a single helper function (with a boolean for nestable vs. non-nestable) and route all calls through that. http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_task_runner.h (right): http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.h:10: #include "base/callback.h" no need for callback.h include (you could include callback_forward.h, but all uses come from inheriting from SequencedTaskRunner, so you can assume it's included in sequenced_task_runner.h) http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.h:15: #include "base/time.h" no need for time.h include (since you're pulling it in via sequenced_task_runner.h) http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.h:18: class Location; no need for this forward-declaration (since you're pulling it in via SequencedTaskRunner) http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.h:23: // Note that this class is RefCountedThreadSafe (inherited from TaskRunner). Add a comment explaining what the class does. Something like: // SequencedWorkerPoolTaskRunner is a SequencedTaskRunner that posts tasks to a SequencedWorkerPool with a fixed sequence token> http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.h:27: SequencedWorkerPool::SequenceToken token); fix indent
+brettw for OWNERS review Brett, Francois and I have gone through a few code review passes already, so this should be quick and easy for you.
https://chromiumcodereview.appspot.com/9663075/diff/37025/base/threading/sequ... File base/threading/sequenced_worker_pool_task_runner.cc (right): https://chromiumcodereview.appspot.com/9663075/diff/37025/base/threading/sequ... base/threading/sequenced_worker_pool_task_runner.cc:37: return pool_->RunsTasksOnCurrentThread(); oh... i hadn't seen this CL until just now... this method impl could be more meaningful if it tested for whether the pool was running under the token_
https://chromiumcodereview.appspot.com/9663075/diff/37025/base/threading/sequ... File base/threading/sequenced_worker_pool_task_runner.cc (right): https://chromiumcodereview.appspot.com/9663075/diff/37025/base/threading/sequ... base/threading/sequenced_worker_pool_task_runner.cc:37: return pool_->RunsTasksOnCurrentThread(); On 2012/03/26 19:57:32, michaeln wrote: > oh... i hadn't seen this CL until just now... this method impl could be more > meaningful if it tested for whether the pool was running under the token_ To be more clear, here's an implementation of the method i'm suggesting would be useful here. https://chromiumcodereview.appspot.com/9845037/
oops, forgot to send out https://chromiumcodereview.appspot.com/9663075/diff/37025/base/threading/sequ... File base/threading/sequenced_worker_pool_task_runner.cc (right): https://chromiumcodereview.appspot.com/9663075/diff/37025/base/threading/sequ... base/threading/sequenced_worker_pool_task_runner.cc:37: return pool_->RunsTasksOnCurrentThread(); On 2012/03/26 19:57:32, michaeln wrote: > oh... i hadn't seen this CL until just now... this method impl could be more > meaningful if it tested for whether the pool was running under the token_ That's a different test, though. The "RunsTasksOnCurrentThread" method is supposed to be for whether the current thread is one that the TaskRunner runs tasks on, not if it's one where the TaskRunner is running a task at this instant. I don't know if we really need this more stringent test (the one in your other CL) -- if you post tasks only via a SequencedWorkerPoolTaskRunner, then it's correct by construction already (from the point of view of the caller).
I realize it's a different test, i think a more useful one. The current method tells you if you're running on a worker pool thread. But that's not very useful to callers that use multiple 'sequences'. They can't distinguish when they're on one sequence vs the another. Making that distinction is useful for at least two different reasons. - DCHECK'ing correctness - conditionally PostTask'ing to do the work under another sequence Afaict, the latter is impossible to do given the current interfaces available on SWP and STR.
On 2012/03/26 22:13:40, michaeln wrote: > I realize it's a different test, i think a more useful one. The current method > tells you if you're running on a worker pool thread. But that's not very useful > to callers that use multiple 'sequences'. They can't distinguish when they're on > one sequence vs the another. Making that distinction is useful for at least two > different reasons. > > - DCHECK'ing correctness > - conditionally PostTask'ing to do the work under another sequence > > Afaict, the latter is impossible to do given the current interfaces available on > SWP and STR. Yeah, I see now. Hmm. Let's move this discussion to your code review. I think I'm warming up to the idea, although I'm trying to think of a cleaner way to implement it...
> move this discussion to your code review. I think I'm warming up to the > idea, although I'm trying to think of a cleaner way to implement it... Great, look forward to your thoughts on that.
Hi Fred, I've addressed your latest comments. Thanks! http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.cc (right): http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.cc:9: #include "base/tracked_objects.h" On 2012/03/26 18:44:08, akalin wrote: > looks like this should be base/location.h (this changed recently) Done. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.cc:15: SequencedTaskTracker::TaskEvent::TaskEvent(int i_in, Type type_in) On 2012/03/26 18:44:08, akalin wrote: > don't use _in, just name them the same; C++'s variable lookup rules are such > that ": i(i), type(type)" works correctly. Done. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.cc:149: for (int i = 0; i < task_count; ++i) { On 2012/03/26 18:44:08, akalin wrote: > This takes n^2 time, which isn't great but probably not worth optimizing. But > should still add a comment about it. Done. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.h (right): http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:33: struct TaskEvent { On 2012/03/26 18:44:08, akalin wrote: > just pull out this struct, since it's already in an internal namespace (and > you're typedefing it anyway) Done. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:35: TaskEvent(int i_in, Type type_in); On 2012/03/26 18:44:08, akalin wrote: > don't use _in, just name them the same Done. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:35: TaskEvent(int i_in, Type type_in); On 2012/03/26 18:44:08, akalin wrote: > define a default constructor and initialize both variables to some default Why define a default constructor if it never gets used? http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:43: void PostWrappedNonNestableTask( On 2012/03/26 18:44:08, akalin wrote: > the index (int i) should be a member of these functions, as otherwise the caller > would have to assume they're sequential Please see my comment right at the bottom. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:83: // Protects events_ and next_post_i_. On 2012/03/26 18:44:08, akalin wrote: > events_ only Please see my comment right at the bottom. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:91: int next_post_i_; On 2012/03/26 18:44:08, akalin wrote: > remove the need for this, since the index should be a member of the post > functions Please see my comment right at the bottom. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:138: task_runner, Bind(&internal::SleepForOneSecond)); On 2012/03/26 18:44:08, akalin wrote: > with the change above to pass i to PostWrappedNonNestableTask, keep around an > expected_post_order vector<int> and push the i's onto that. Then assert that > it's the same as the actual post order. (And do something similar for other > tests.) Please see my comment right at the bottom. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:219: children_per_parent); On 2012/03/26 18:44:08, akalin wrote: > probably would have to pass in a start/end index for the child tasks, too This won't work for tests (such as NonNestablePostFromNonNestableTask) in which some tasks are posted from other tasks. In those cases the posts happen on different threads, and therefore the final post order cannot be predicted at this point in the code. I could have separate post functions for the different test types, but I thought it would be best to have all tests use the same code. http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_task_runner.cc (right): http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.cc:26: return pool_->PostSequencedWorkerTask(token_, from_here, task); On 2012/03/26 18:44:08, akalin wrote: > you want a DCHECK_EQ(delay_ms, 0) here, too, like below Done. http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.cc:33: return pool_->PostSequencedWorkerTask(token_, from_here, task); On 2012/03/26 18:44:08, akalin wrote: > here, too. In fact, you may just want to have a single helper function (with a > boolean for nestable vs. non-nestable) and route all calls through that. Done. http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_task_runner.h (right): http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.h:10: #include "base/callback.h" On 2012/03/26 18:44:08, akalin wrote: > no need for callback.h include (you could include callback_forward.h, but all > uses come from inheriting from SequencedTaskRunner, so you can assume it's > included in sequenced_task_runner.h) Done. http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.h:15: #include "base/time.h" On 2012/03/26 18:44:08, akalin wrote: > no need for time.h include (since you're pulling it in via > sequenced_task_runner.h) Yeah but the Google style guide says not to rely on other headers to pull in definitions. Is this situation different because SWPTR derives from STR? http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.h:18: class Location; On 2012/03/26 18:44:08, akalin wrote: > no need for this forward-declaration (since you're pulling it in via > SequencedTaskRunner) Please see my comment above. http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.h:23: // Note that this class is RefCountedThreadSafe (inherited from TaskRunner). On 2012/03/26 18:44:08, akalin wrote: > Add a comment explaining what the class does. Something like: > > // SequencedWorkerPoolTaskRunner is a SequencedTaskRunner that posts tasks to a > SequencedWorkerPool with a fixed sequence token> Done. http://codereview.chromium.org/9663075/diff/37025/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.h:27: SequencedWorkerPool::SequenceToken token); On 2012/03/26 18:44:08, akalin wrote: > fix indent Done.
Okay, still LGTM. You may want to ping brettw directly to get OWNERS approval. http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... File base/test/sequenced_task_runner_test_template.h (right): http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... base/test/sequenced_task_runner_test_template.h:35: TaskEvent(int i_in, Type type_in); On 2012/03/28 14:32:42, Francois wrote: > On 2012/03/26 18:44:08, akalin wrote: > > define a default constructor and initialize both variables to some default > > Why define a default constructor if it never gets used? Just defensive coding; the same reason why it's suggested (in the style guide I think) to initialize variables to some value instead of leaving them undefined. Besides, if you're putting the struct in an STL container, the default constructor *is* probably being used internally.
On 2012/03/29 18:44:37, akalin wrote: > Okay, still LGTM. You may want to ping brettw directly to get OWNERS approval. Will do, and thanks! > > http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... > File base/test/sequenced_task_runner_test_template.h (right): > > http://codereview.chromium.org/9663075/diff/37025/base/test/sequenced_task_ru... > base/test/sequenced_task_runner_test_template.h:35: TaskEvent(int i_in, Type > type_in); > On 2012/03/28 14:32:42, Francois wrote: > > On 2012/03/26 18:44:08, akalin wrote: > > > define a default constructor and initialize both variables to some default > > > > Why define a default constructor if it never gets used? > > Just defensive coding; the same reason why it's suggested (in the style guide I > think) to initialize variables to some value instead of leaving them undefined. > > Besides, if you're putting the struct in an STL container, the default > constructor *is* probably being used internally. OK, but the default constructor only gets generated if there are no user-defined constructors, so in this case any code which invokes the default constructor would cause a compilation error.
LGTM based on Fred's review. http://codereview.chromium.org/9663075/diff/44001/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool_task_runner.cc (right): http://codereview.chromium.org/9663075/diff/44001/base/threading/sequenced_wo... base/threading/sequenced_worker_pool_task_runner.cc:6: Nit: remove extra blank line.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/francoisk777@gmail.com/9663075/54001
Change committed as 130113 |
