Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9924023:
Readability review (Closed)
|
Created:
8 years, 8 months ago by szym Modified:
8 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[net] Update style in PriorityQueue and PrioritizedDispatcher after readability review.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140133
Patch Set 1 #
Total comments: 47
Patch Set 2 : Rebased to master (after change of RequestPriority ordering). #Patch Set 3 : Responded to review. #
Total comments: 8
Patch Set 4 : Responded to review. #Patch Set 5 : Added death tests. #Patch Set 6 : Added compiler conditions for death tests. #Patch Set 7 : Add copy constructor and assignment operator to Pointer. #
Total comments: 4
Patch Set 8 : Responded to review. #
Total comments: 15
Patch Set 9 : Responded to Matt's comments. #
Total comments: 2
Patch Set 10 : Simpler comments. #
Messages
Total messages: 19 (0 generated)
http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher.h File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:16: // A priority-based dispatcher of jobs. Dispatch order is by priority (lowest If it's possible to reverse the decision of lowest number == highest priority, I would suggest doing so. You'll have to explain that everywhere you use priorities, and it's very easy to get wrong, as it's unnatural to read code like this: if (priority1 < priority2) { // priority one is higher priority, not lower } Basically you're introducing cognitive load every time priorities are used. Another way around that is to explicitly define some readable interface to the priorities which does the right thing with the numbers, eg if (IsHigherPriority(priority1, priority2)) http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:23: // Job callbacks. However, this class is NOT thread-safe, which is enforced It's not clear what all this means. Perhaps give some examples of what is safe or not safe to do, or document it down at the method level. eg, it's very unclear to me how one can safely call Cancel (especially if OnJobFinished might be called from some other thread). http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:35: // at priority 1 or 0, but the rest has to be at 0. Can you explain this comment more, it doesn't make sense to me. Perhaps there's some assumptions about how the priorities work that are not stated here; or simply a typo. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:53: // Note: PriorityDispatch will never delete a Job. What is PriorityDispatch? http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:56: // done. When done what? done starting? http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:65: PrioritizedDispatcher(const Limits& limits); Single arg constructors must be declared explicit. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:76: Handle Add(Job* job, Priority priority); Document ownership semantics when passing mutable pointers. Also, fix parameter ordering. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... File net/base/prioritized_dispatcher_unittest.cc (right): http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:30: // A job that appends |data| to |log_| when started and '.' when finished. So that means it's ambiguous in the log which job is finishing. You might consider having it append +data -data on start/finish instead. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:35: TestJob(PrioritizedDispatcherTest* test, char data, Priority priority) Fix parameter order. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:38: // MSVS does not accept EXPECT_EQ(this, ...) so wrap it up. This doesn't seem so useful to me. If any expectation involving this actually triggered, how would you debug the problem? (it would take some effort just to understand what was broken, as it's just printing out memory addresses? unless there's some magic in EXPECT_EQ to do better, though I don't really see how there could be). I'd suggest instead just adding an accessor for data_, and comparing those when needed. Then your error messages will be a lot more understandable when they fire. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:44: EXPECT_TRUE(handle_.is_null()); Many of these expectations do not depend on the code under test, they would only fire if the code of the test itself is incorrect. For these cases, use CHECK rather than EXPECT. Only use EXPECT/ASSERT for incorrect behavior of the code you're actually testing. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:114: PrioritizedDispatcherTest* test_; This seems a bit sketchy to me (it sets up circular dependencies among these classes). Why not just pass in pointers to the log and dispatcher; or a pointer to some struct containing them, which the test then owns. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:143: scoped_ptr<PrioritizedDispatcher> dispatch_; dispatcher_ http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:275: What about cancelling a null, or not-in-queue handle (eg handle from a different queue). What about evicting when the queue is empty? http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h#newco... net/base/priority_queue.h:37: typedef std::list<std::pair<size_t, T> > List; why size_t for the id? it's not a size. http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h#newco... net/base/priority_queue.h:50: Pointer() : priority_(kNullPriority) { If you intend to make this copyable, define the copy constructor and assignment operator; don't make maintainers wonder whether you just forgot to add DISALLOW_COPY_AND_ASSIGN or not. http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h#newco... net/base/priority_queue.h:102: explicit PriorityQueue(Priority num_priorities) Similarly, DISALLOW_COPY_OR_ASSIGN, or define copy constructor and assignment operator. (if you do make copyable, you should note what happens to Pointer's). http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h#newco... net/base/priority_queue.h:146: DCHECK(CalledOnValidThread()); For all these methods - is it worth checking size_ == 0 first? http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue_unittes... File net/base/priority_queue_unittest.cc (right): http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue_unittes... net/base/priority_queue_unittest.cc:50: PriorityQueue<int> queue(kNumPriorities); Why not factor out a fixture here with this common setup that you do in every test. While it doesn't save much code, it makes it explicit that you're doing the same setup on each test, so maintainers don't have to scrutinize to see what's going on.
Thank you very much for performing this review. Sorry for the long delay. Changing RequestPriority ordering throughout src/net took some time, but I'm back on track. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher.h File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:16: // A priority-based dispatcher of jobs. Dispatch order is by priority (lowest On 2012/04/11 16:13:58, stevez wrote: > If it's possible to reverse the decision of lowest number == highest priority, I > would suggest doing so. You'll have to explain that everywhere you use > priorities, and it's very easy to get wrong, as it's unnatural to read code like > this: > if (priority1 < priority2) { > // priority one is higher priority, not lower > } > Basically you're introducing cognitive load every time priorities are used. > > Another way around that is to explicitly define some readable interface to the > priorities which does the right thing with the numbers, eg > if (IsHigherPriority(priority1, priority2)) > Took some work, but done. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:23: // Job callbacks. However, this class is NOT thread-safe, which is enforced On 2012/04/11 16:13:58, stevez wrote: > It's not clear what all this means. > > Perhaps give some examples of what is safe or not safe to do, or document it > down at the method level. eg, it's very unclear to me how one can safely call > Cancel (especially if OnJobFinished might be called from some other thread). That's not allowed. The class is NOT thread-safe. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:35: // at priority 1 or 0, but the rest has to be at 0. On 2012/04/11 16:13:58, stevez wrote: > Can you explain this comment more, it doesn't make sense to me. Perhaps there's > some assumptions about how the priorities work that are not stated here; or > simply a typo. There's no typo, I updated the comment after priority reordering. Is it still unclear? http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:53: // Note: PriorityDispatch will never delete a Job. On 2012/04/11 16:13:58, stevez wrote: > What is PriorityDispatch? Done. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:56: // done. On 2012/04/11 16:13:58, stevez wrote: > When done what? done starting? Fixed wording. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:65: PrioritizedDispatcher(const Limits& limits); On 2012/04/11 16:13:58, stevez wrote: > Single arg constructors must be declared explicit. Done. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:76: Handle Add(Job* job, Priority priority); On 2012/04/11 16:13:58, stevez wrote: > Document ownership semantics when passing mutable pointers. Done. > Also, fix parameter ordering. I prefer to leave this one as is. This is a very common pattern in Chromium codebase: http://goo.gl/4lnej (in src/net alone). http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... File net/base/prioritized_dispatcher_unittest.cc (right): http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:30: // A job that appends |data| to |log_| when started and '.' when finished. On 2012/04/11 16:13:58, stevez wrote: > So that means it's ambiguous in the log which job is finishing. You might > consider having it append +data -data on start/finish instead. The intention here is the the order of jobs finishing is not relevant for the correctness of PrioritizedDispatcher. I've updated EnforceLimits to make this point. I've initially written +/- and Aa, but found it overall less readable. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:35: TestJob(PrioritizedDispatcherTest* test, char data, Priority priority) On 2012/04/11 16:13:58, stevez wrote: > Fix parameter order. As before, I prefer to leave this one as is. This is a very common pattern in Chromium codebase: http://goo.gl/4lnej Even though the pointer is mutable, we don't consider it "output". http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:38: // MSVS does not accept EXPECT_EQ(this, ...) so wrap it up. On 2012/04/11 16:13:58, stevez wrote: > This doesn't seem so useful to me. If any expectation involving this actually > triggered, how would you debug the problem? (it would take some effort just to > understand what was broken, as it's just printing out memory addresses? unless > there's some magic in EXPECT_EQ to do better, though I don't really see how > there could be). > > I'd suggest instead just adding an accessor for data_, and comparing those when > needed. Then your error messages will be a lot more understandable when they > fire. Done. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:44: EXPECT_TRUE(handle_.is_null()); On 2012/04/11 16:13:58, stevez wrote: > Many of these expectations do not depend on the code under test, they would only > fire if the code of the test itself is incorrect. For these cases, use CHECK > rather than EXPECT. Only use EXPECT/ASSERT for incorrect behavior of the code > you're actually testing. I generally agree. However, a PrioritizedDispatcher which starts the jobs in faulty order or incorrectly returns null handle will lead to crash in subsequent calls to ChangePriority and Cancel. To make DCHECK safe, we'd need an ASSERT in the test case body. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:114: PrioritizedDispatcherTest* test_; On 2012/04/11 16:13:58, stevez wrote: > This seems a bit sketchy to me (it sets up circular dependencies among these > classes). Why not just pass in pointers to the log and dispatcher; or a pointer > to some struct containing them, which the test then owns. Done. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:143: scoped_ptr<PrioritizedDispatcher> dispatch_; On 2012/04/11 16:13:58, stevez wrote: > dispatcher_ Done. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:275: On 2012/04/11 16:13:58, stevez wrote: > What about cancelling a null, or not-in-queue handle (eg handle from a different > queue). Not legal. Will crash on DCHECK. > What about evicting when the queue is empty? Added.
None of the comments in priority_queue* were addressed. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher.h File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:35: // at priority 1 or 0, but the rest has to be at 0. On 2012/05/14 22:56:02, szym wrote: > On 2012/04/11 16:13:58, stevez wrote: > > Can you explain this comment more, it doesn't make sense to me. Perhaps > there's > > some assumptions about how the priorities work that are not stated here; or > > simply a typo. > > There's no typo, I updated the comment after priority reordering. Is it still > unclear? The typo was 'at priority 1 or 0' -> 'at priority 1 or 2'. The new comment is fine. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... File net/base/prioritized_dispatcher_unittest.cc (right): http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:44: EXPECT_TRUE(handle_.is_null()); On 2012/05/14 22:56:02, szym wrote: > On 2012/04/11 16:13:58, stevez wrote: > > Many of these expectations do not depend on the code under test, they would > only > > fire if the code of the test itself is incorrect. For these cases, use CHECK > > rather than EXPECT. Only use EXPECT/ASSERT for incorrect behavior of the code > > you're actually testing. > > I generally agree. However, a PrioritizedDispatcher which starts the jobs in > faulty order or incorrectly returns null handle will lead to crash in subsequent > calls to ChangePriority and Cancel. To make DCHECK safe, we'd need an ASSERT in > the test case body. I was suggesting CHECK for a reason, not DCHECK. (really, I can't think of a reason to use DCHECK in test code, you can always crash in test code!). If you're concerned about a null handle being returned, ASSERT appropriately when you receive it from the dispatcher. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:275: On 2012/05/14 22:56:02, szym wrote: > On 2012/04/11 16:13:58, stevez wrote: > > What about cancelling a null, or not-in-queue handle (eg handle from a > different > > queue). > > Not legal. Will crash on DCHECK. Great, why not add a death test for that? > > > What about evicting when the queue is empty? > > Added. http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:22: // reentrant: it is safe to execute any method (incl. destructor) from within Ok, let me be more clear: can you provide an example of how to take advantage of the fact that this class is re-entrant, but not thread-safe. I have never encountered code that claims to be re-entrant but not thread-safe (and most discussion I've found online, people assume all reentrant code IS threadsafe, the only non-example of it is in a wikipedia article just showing you can actually write such). I don't know why it would be useful. I certainly don't know why you would highlight the re-entrant property over the NOT thread-safe property. To be more specific - in order to make a re-entrant call, I must have called you, and you called me back somehow before that call returned, and now I can call you back. If you're not thread-safe, either I'm using a mutex (in which case, why would I care at all about re-entrancy?), or I know it's all in the same thread, which seems like a pretty huge encapsulation violation (ie I'm assuming things about how you called me back). From a readability perspective, it seems this talk of reentrancy is beside the point and simply confusing; just say it's not thread-safe and leave it at that. If there's some case where reentrancy is important, mention it AFTER saying it's not threadsafe (non-thread-safety seems hugely more important), and explain why it matters because for the average engineer it won't be at all obvious. In fact, I'd wager the average engineer will assume it's a typo in the comment (if they even bother to pay attention to it)! http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:77: Handle Add(Job* job, Priority priority); parameter ordering still needs to be fixed
Thanks for review. I missed your previous comments on priority_queue*. Responded now. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher.h File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher.h:35: // at priority 1 or 0, but the rest has to be at 0. On 2012/05/17 17:57:07, stevez wrote: > On 2012/05/14 22:56:02, szym wrote: > > On 2012/04/11 16:13:58, stevez wrote: > > > Can you explain this comment more, it doesn't make sense to me. Perhaps > > there's > > > some assumptions about how the priorities work that are not stated here; or > > > simply a typo. > > > > There's no typo, I updated the comment after priority reordering. Is it still > > unclear? > > The typo was 'at priority 1 or 0' -> 'at priority 1 or 2'. The new comment is > fine. That was not a typo. That one slot could be used by priority 1 or "higher" (0). Fortunately, the higher/lower confusion is gone now. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... File net/base/prioritized_dispatcher_unittest.cc (right): http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:44: EXPECT_TRUE(handle_.is_null()); On 2012/05/17 17:57:07, stevez wrote: > On 2012/05/14 22:56:02, szym wrote: > > On 2012/04/11 16:13:58, stevez wrote: > > > Many of these expectations do not depend on the code under test, they would > > only > > > fire if the code of the test itself is incorrect. For these cases, use > CHECK > > > rather than EXPECT. Only use EXPECT/ASSERT for incorrect behavior of the > code > > > you're actually testing. > > > > I generally agree. However, a PrioritizedDispatcher which starts the jobs in > > faulty order or incorrectly returns null handle will lead to crash in > subsequent > > calls to ChangePriority and Cancel. To make DCHECK safe, we'd need an ASSERT > in > > the test case body. > > I was suggesting CHECK for a reason, not DCHECK. (really, I can't think of a > reason to use DCHECK in test code, you can always crash in test code!). > > If you're concerned about a null handle being returned, ASSERT appropriately > when you receive it from the dispatcher. An ASSERT_ within TestJob::Add or TestJob::ChangePriority will not quit the test. I added ASSERTs to test body. http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:275: On 2012/05/17 17:57:07, stevez wrote: > On 2012/05/14 22:56:02, szym wrote: > > On 2012/04/11 16:13:58, stevez wrote: > > > What about cancelling a null, or not-in-queue handle (eg handle from a > > different > > > queue). > > > > Not legal. Will crash on DCHECK. > > Great, why not add a death test for that? > > > > > > What about evicting when the queue is empty? > > > > Added. > I can't find an example of a death test in Chromium. From sheriffing experience, tests which fail with CHECKs or DCHECKs typically end up disabled. http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h#newco... net/base/priority_queue.h:37: typedef std::list<std::pair<size_t, T> > List; On 2012/04/11 16:13:58, stevez wrote: > why size_t for the id? it's not a size. Done. http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h#newco... net/base/priority_queue.h:50: Pointer() : priority_(kNullPriority) { On 2012/04/11 16:13:58, stevez wrote: > If you intend to make this copyable, define the copy constructor and assignment > operator; don't make maintainers wonder whether you just forgot to add > DISALLOW_COPY_AND_ASSIGN or not. The default copy constructor and assignment are fine. Would an explicit comment suffice? http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h#newco... net/base/priority_queue.h:102: explicit PriorityQueue(Priority num_priorities) On 2012/04/11 16:13:58, stevez wrote: > Similarly, DISALLOW_COPY_OR_ASSIGN, or define copy constructor and assignment > operator. > (if you do make copyable, you should note what happens to Pointer's). Done. http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h#newco... net/base/priority_queue.h:146: DCHECK(CalledOnValidThread()); On 2012/04/11 16:13:58, stevez wrote: > For all these methods - is it worth checking size_ == 0 first? In the original review it was considered unnecessary. http://codereview.chromium.org/8965025/#msg16 "It would be clearer to just remove this line and the NOTREACHED statement below - I'm not too worried about perf here. Ditto for the rest of this file." http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue_unittes... File net/base/priority_queue_unittest.cc (right): http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue_unittes... net/base/priority_queue_unittest.cc:50: PriorityQueue<int> queue(kNumPriorities); On 2012/04/11 16:13:58, stevez wrote: > Why not factor out a fixture here with this common setup that you do in every > test. While it doesn't save much code, it makes it explicit that you're doing > the same setup on each test, so maintainers don't have to scrutinize to see > what's going on. Done. http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:22: // reentrant: it is safe to execute any method (incl. destructor) from within On 2012/05/17 17:57:07, stevez wrote: > Ok, let me be more clear: can you provide an example of how to take advantage > of the fact that this class is re-entrant, but not thread-safe. Understood. I should remove "re-entrant" and leave the explicit statement: "It is safe to execute any method, including destructor, from within a callback." after highlighting it's non thread safe. > To be more specific - in order to make a re-entrant call, I must have called > you, and you called me back somehow before that call returned, and now I can > call you back. That's exactly the scenario. With callback passing there is often the case when we want to take action on the caller. For example, a Job that has been started might want to add another Job to the PrioritizedDispatcher, or it might destroy it. Consider: struct QuitJob : public Job { virtual void Start() { delete dispatcher; } PrioritizedDispatcher* dispatcher; }; PrioritizedDispatcher* dispatcher = ... QuitJob quit = { dispatcher; } dispatcher->AddJob(&quit, IDLE); // AddJob calls QuitJob::Start // which deletes dispatcher We don't normally guarantee that it's safe to act on the caller from the callback. In most cases, this is enabled by posting the callback to a MessageLoop, so that the caller is the MessageLoop. In this case, by careful design, it's safe to call on PrioritizedDispatcher from Job::Start. > If you're not thread-safe, either I'm using a mutex (in which > case, why would I care at all about re-entrancy?), or I know it's all in the > same thread, which seems like a pretty huge encapsulation violation (ie I'm > assuming things about how you called me back). In Chromium's net this is the common assumption (that it's on the single thread). Classes which are thread-safe are typically explicitly documented as such. Classes are only explicitly documented as NonThreadSafe when there is reasonable concern that different threads might incorrectly call into them. http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:77: Handle Add(Job* job, Priority priority); On 2012/05/17 17:57:07, stevez wrote: > parameter ordering still needs to be fixed I see the second part of my comment does not show up in Rietveld unless expanded: "I prefer to leave this one as is. This is a very common pattern in Chromium codebase: http://goo.gl/4lnej (in src/net alone)." |job| is hardly an output argument. Add does not write to |job|. I would normally use a Callback (essentially passed by value), but EvictOldestLowest requires using something identifiable.
http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... File net/base/prioritized_dispatcher_unittest.cc (right): http://codereview.chromium.org/9924023/diff/1/net/base/prioritized_dispatcher... net/base/prioritized_dispatcher_unittest.cc:275: On 2012/05/17 19:52:01, szym wrote: > On 2012/05/17 17:57:07, stevez wrote: > > On 2012/05/14 22:56:02, szym wrote: > > > On 2012/04/11 16:13:58, stevez wrote: > > > > What about cancelling a null, or not-in-queue handle (eg handle from a > > > different > > > > queue). > > > > > > Not legal. Will crash on DCHECK. > > > > Great, why not add a death test for that? > > > > > > > > > What about evicting when the queue is empty? > > > > > > Added. > > > > I can't find an example of a death test in Chromium. From sheriffing experience, > tests which fail with CHECKs or DCHECKs typically end up disabled. Forgive my ignorance. I now added a death test.
http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h#newco... net/base/priority_queue.h:50: Pointer() : priority_(kNullPriority) { On 2012/05/17 19:52:01, szym wrote: > On 2012/04/11 16:13:58, stevez wrote: > > If you intend to make this copyable, define the copy constructor and > assignment > > operator; don't make maintainers wonder whether you just forgot to add > > DISALLOW_COPY_AND_ASSIGN or not. > > The default copy constructor and assignment are fine. Would an explicit comment > suffice? http://www.corp.google.com/eng/doc/cppguide.xml?showone=Copy_Constructors#Cop... "If a copy method is insufficient in your situation (e.g. for performance reasons, or because your class needs to be stored by value in an STL container), provide both a copy constructor and assignment operator." http://www.corp.google.com/eng/doc/devguide/cpp/primer.html#disallow_copy_and... "Any user-defined class that does not have value semantics (i.e., one that cannot be safely assigned or copy-constructed) must either redefine operator= and a copy constructor or hide those two methods so that the compiler does not implicitly generate incorrect implementations of those behaviours." http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:77: Handle Add(Job* job, Priority priority); On 2012/05/17 19:52:01, szym wrote: > On 2012/05/17 17:57:07, stevez wrote: > > parameter ordering still needs to be fixed > > I see the second part of my comment does not show up in Rietveld unless > expanded: > > "I prefer to leave this one as is. This is a very common pattern in Chromium > codebase: http://goo.gl/4lnej (in src/net alone)." > > |job| is hardly an output argument. Add does not write to |job|. I would > normally use a Callback (essentially passed by value), but EvictOldestLowest > requires using something identifiable. Ok on the parameter order, but can you explain what you mean here about using a callback? If you don't need a job class at all it'd be good to get rid of it. I don't see what you mean by EvictOldestLowest needing something identifiable. (not least because the Job interface doesn't provide any more identifiability than a callback does).
http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/9924023/diff/1/net/base/priority_queue.h#newco... net/base/priority_queue.h:50: Pointer() : priority_(kNullPriority) { On 2012/05/22 14:22:40, stevez wrote: > On 2012/05/17 19:52:01, szym wrote: > > On 2012/04/11 16:13:58, stevez wrote: > > > If you intend to make this copyable, define the copy constructor and > > assignment > > > operator; don't make maintainers wonder whether you just forgot to add > > > DISALLOW_COPY_AND_ASSIGN or not. > > > > The default copy constructor and assignment are fine. Would an explicit > comment > > suffice? > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=Copy_Constructors#Cop... > > "If a copy method is insufficient in your situation (e.g. for performance > reasons, or because your class needs to be stored by value in an STL container), > provide both a copy constructor and assignment operator." > > http://www.corp.google.com/eng/doc/devguide/cpp/primer.html#disallow_copy_and... > > "Any user-defined class that does not have value semantics (i.e., one that > cannot be safely assigned or copy-constructed) must either redefine operator= > and a copy constructor or hide those two methods so that the compiler does not > implicitly generate incorrect implementations of those behaviours." Understood. Done. http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:77: Handle Add(Job* job, Priority priority); On 2012/05/22 14:22:40, stevez wrote: > On 2012/05/17 19:52:01, szym wrote: > > On 2012/05/17 17:57:07, stevez wrote: > > > parameter ordering still needs to be fixed > > > > I see the second part of my comment does not show up in Rietveld unless > > expanded: > > > > "I prefer to leave this one as is. This is a very common pattern in Chromium > > codebase: http://goo.gl/4lnej (in src/net alone)." > > > > |job| is hardly an output argument. Add does not write to |job|. I would > > normally use a Callback (essentially passed by value), but EvictOldestLowest > > requires using something identifiable. > > Ok on the parameter order, but can you explain what you mean here about using a > callback? If you don't need a job class at all it'd be good to get rid of it. > I don't see what you mean by EvictOldestLowest needing something identifiable. > (not least because the Job interface doesn't provide any more identifiability > than a callback does). > PrioritizedDispatcher::Job has only one method, so the preferred solution is to use Callback (see: http://code.google.com/searchframe#OAMlx_jo-ck/src/base/callback.h ). However, in some use cases PrioritizedDispatcher::EvictOldestLowest only provides utility if the caller can identify the Job (or Callback) that has been evicted. See: http://code.google.com/searchframe#OAMlx_jo-ck/src/net/base/host_resolver_imp... Since Callback is passed by value (const& for efficiency only), it would not provide such identification.
http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:77: Handle Add(Job* job, Priority priority); On 2012/05/23 01:26:49, szym wrote: > On 2012/05/22 14:22:40, stevez wrote: > > On 2012/05/17 19:52:01, szym wrote: > > > On 2012/05/17 17:57:07, stevez wrote: > > > > parameter ordering still needs to be fixed > > > > > > I see the second part of my comment does not show up in Rietveld unless > > > expanded: > > > > > > "I prefer to leave this one as is. This is a very common pattern in Chromium > > > codebase: http://goo.gl/4lnej (in src/net alone)." > > > > > > |job| is hardly an output argument. Add does not write to |job|. I would > > > normally use a Callback (essentially passed by value), but EvictOldestLowest > > > requires using something identifiable. > > > > Ok on the parameter order, but can you explain what you mean here about using > a > > callback? If you don't need a job class at all it'd be good to get rid of it. > > > I don't see what you mean by EvictOldestLowest needing something identifiable. > > > (not least because the Job interface doesn't provide any more identifiability > > than a callback does). > > > > PrioritizedDispatcher::Job has only one method, so the preferred solution is to > use Callback (see: > http://code.google.com/searchframe#OAMlx_jo-ck/src/base/callback.h ). However, > in some use cases PrioritizedDispatcher::EvictOldestLowest only provides utility > if the caller can identify the Job (or Callback) that has been evicted. See: > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/base/host_resolver_imp... > > Since Callback is passed by value (const& for efficiency only), it would not > provide such identification. Does chromium use a different callback library than google3? You can certainly pass callbacks around by pointer, that is the standard way of doing so. The usage you point to requires casting, which is unfortunate. You might enrich the job interface instead. Or, you might return a Handle from EvictOldestLowest as a different mechanism to match up its return value to a corresponding Add call (assuming it has suitable semantics, I didn't look; at any rate it seems reasonable that the job manipulation methods are all in terms of handles). In any case this is advisory only, feel free to change or not. http://codereview.chromium.org/9924023/diff/28002/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/28002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:5: #ifndef NET_BASE_PRIORITY_DISPATCH_H_ Should be DISPATCHER throughout http://codereview.chromium.org/9924023/diff/28002/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/9924023/diff/28002/net/base/priority_queue.h#n... net/base/priority_queue.h:63: priority_ = p.priority_; You may want to check for self-assignment.
http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/11007/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:77: Handle Add(Job* job, Priority priority); On 2012/05/30 17:12:45, stevez wrote: > Does chromium use a different callback library than google3? You can certainly > pass callbacks around by pointer, that is the standard way of doing so. Yes, Chromium uses different callback library. If curious, see callback.h: http://code.google.com/searchframe#OAMlx_jo-ck/src/base/callback.h > The usage you point to requires casting, which is unfortunate. You might enrich > the job interface instead. In the original review, we found that calling Job::OnEvicted from PrioritizedDispatcher is awkward because the Job could be evicted on addition. We decided that queue limits and eviction are better handled at the caller (HostResolverImpl). > Or, you might return a Handle from EvictOldestLowest > as a different mechanism to match up its return value to a corresponding Add PriorityQueue::Pointer is only valid for objects still in the queue. But more importantly, Handle::value() returns the generic PrioritizedDispatcher::Job, so static_cast is inevitable unless PrioritizedDispatcher becomes a template. http://codereview.chromium.org/9924023/diff/28002/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/28002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:5: #ifndef NET_BASE_PRIORITY_DISPATCH_H_ On 2012/05/30 17:12:45, stevez wrote: > Should be DISPATCHER throughout Done. http://codereview.chromium.org/9924023/diff/28002/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/9924023/diff/28002/net/base/priority_queue.h#n... net/base/priority_queue.h:63: priority_ = p.priority_; On 2012/05/30 17:12:45, stevez wrote: > You may want to check for self-assignment. Self-assignment is benign. Added comment.
http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:33: // allow for at most 30 running jobs in total. If there are already 24 jobs optional nit: Suggest you add ", and no jobs at priority 0 will ever be started", After "in total". While higher priority -> more important may be intuitive, first item in a list is the highest priority is also intuitive, so the comment draws a little more attention to the fact that lowest priority is first in the list. http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:76: // started. The dispatcher does not own |job|, but must be outlived by |job|. Shouldn't this be "but must outlive |job|"? http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:117: nit: Remove extra blank line (Goes for other files, too) http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... File net/base/prioritized_dispatcher_unittest.cc (right): http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher_unittest.cc:57: CHECK(!running_); Hmm...Wonder about using CHECKs here. CHECKs will abort the entire set of net checks, so regressions in other tests can be masked, if the occur in the same set of CLs. Not going to hold up the CL on it, but wonder about best practices here. http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher_unittest.cc:229: ASSERT_TRUE(job_g->running()); These should probably be in the same order as below. Why did you reoderer the jobs below, anyways? http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher_unittest.cc:239: job_e->Finish(); // Releases nit: // Releases c http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher_unittest.cc:359: #endif // GTEST_HAS_DEATH_TEST && !defined(NDEBUG) nit: Two spaces before comment.
http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:33: // allow for at most 30 running jobs in total. If there are already 24 jobs On 2012/06/01 17:45:34, Matt Menke wrote: > optional nit: Suggest you add ", and no jobs at priority 0 will ever be > started", After "in total". Those are reserved slots. Only 20 slots are reserved, so there could be up to 10 jobs at priority 0. I added comment. http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:76: // started. The dispatcher does not own |job|, but must be outlived by |job|. On 2012/06/01 17:45:34, Matt Menke wrote: > Shouldn't this be "but must outlive |job|"? The Job must outlive the dispatcher so that it can be manipulated by it. I clarified the comment. http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:117: On 2012/06/01 17:45:34, Matt Menke wrote: > nit: Remove extra blank line (Goes for other files, too) Done. http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... File net/base/prioritized_dispatcher_unittest.cc (right): http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher_unittest.cc:57: CHECK(!running_); On 2012/06/01 17:45:34, Matt Menke wrote: > Hmm...Wonder about using CHECKs here. CHECKs will abort the entire set of net > checks, so regressions in other tests can be masked, if the occur in the same > set of CLs. > > Not going to hold up the CL on it, but wonder about best practices here. The rationale was that faults in the test should be caught by CHECKs while faults in the tested class should be caught by ASSERT/EXPECT. In this test suite, Add is called only once, so there's no risk, but if a Job were to be re-Added, one should ASSERT_ that it's not running before doing so. http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher_unittest.cc:229: ASSERT_TRUE(job_g->running()); On 2012/06/01 17:45:34, Matt Menke wrote: > These should probably be in the same order as below. Why did you reoderer the > jobs below, anyways? This is purposeful. "Finish them in any order". The order of jobs finishing does not matter for the dispatcher.
http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:33: // allow for at most 30 running jobs in total. If there are already 24 jobs On 2012/06/01 18:15:01, szym wrote: > On 2012/06/01 17:45:34, Matt Menke wrote: > > optional nit: Suggest you add ", and no jobs at priority 0 will ever be > > started", After "in total". > > Those are reserved slots. Only 20 slots are reserved, so there could be up to 10 > jobs at priority 0. I added comment. Ah, right, I forgot about the 10. http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:76: // started. The dispatcher does not own |job|, but must be outlived by |job|. On 2012/06/01 18:15:01, szym wrote: > On 2012/06/01 17:45:34, Matt Menke wrote: > > Shouldn't this be "but must outlive |job|"? > > The Job must outlive the dispatcher so that it can be manipulated by it. I > clarified the comment. But you say Jobs must call OnJobFinished after being started, and they're generally deleted at some point after that, so they don't outlive the dispatcher. Maybe "|job| must outlive the dispatcher unless cancelled or started?"
http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/34002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:76: // started. The dispatcher does not own |job|, but must be outlived by |job|. On 2012/06/01 18:30:05, Matt Menke wrote: > On 2012/06/01 18:15:01, szym wrote: > > On 2012/06/01 17:45:34, Matt Menke wrote: > > > Shouldn't this be "but must outlive |job|"? > > > > The Job must outlive the dispatcher so that it can be manipulated by it. I > > clarified the comment. > > But you say Jobs must call OnJobFinished after being started, and they're > generally deleted at some point after that, so they don't outlive the > dispatcher. > > Maybe "|job| must outlive the dispatcher unless cancelled or started?" Err...Or evicted. Ok...Maybe "|jobs| that are still queued cannot be deleted until the dispatcher is destroyed or they are no longer queued?"
http://codereview.chromium.org/9924023/diff/39002/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/9924023/diff/39002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:79: // it is started, canceled or evicted. Changed to: "The dispatcher does not own |job|, but |job| must live as long as it is queued in the dispatcher." I think the comment at |class Job| should suffice. http://codereview.chromium.org/9924023/diff/39002/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:86: // Removes and returns the oldest-lowest Job from the queue invalidating any Changed to "Cancels and returns".
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/9924023/40008
Try job failure for 9924023-40008 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/9924023/40008
Change committed as 140133 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
