Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8965025:
Refactoring of job dispatch in HostResolverImpl in preparation for DnsClient. (Closed)
|
Created:
9 years ago by szym Modified:
8 years, 11 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactoring of job dispatch in HostResolverImpl in preparation for DnsClient.
- Adds PrioritizedDispatcher (and PriorityQueue) to replace
HostResolverImpl::JobPool.
- Separates Job which aggregates Requests from ProcTask which executes on
WorkerPool.
- Queues Jobs rather than Requests before dispatching them.
(So that we can queue before DnsClient and then before ProcTask.)
This is the second CL in series to merge AsyncHostResolver into
HostResolverImpl.
BUG=107880
TEST=waterfall
Patch Set 1 : Re-upload #Patch Set 2 : Added NET_EXPORT_PRIVATE for win_Shared. #Patch Set 3 : Removed NET_EXPORT_PRIVATE from template. #
Total comments: 86
Patch Set 4 : Fixed PriorityQueue. #
Total comments: 38
Patch Set 5 : Addressed review. Added WouldEvict to simplify Job lifetime. Renamed to PrioritizedDispatcher #Patch Set 6 : Completed renaming ProcJob->ProcTask. Moved priority_dispatch* to priority_dispatcher*' #Patch Set 7 : Rebased to trunk. #
Total comments: 20
Patch Set 8 : Moved eviction logic from PrioritizedDispatcher to HostResolverImpl #Patch Set 9 : Decoupled PriorityQueue and PrioritizedDispatcher from RequestPriority. #Patch Set 10 : Added validity check to PriorityQueue::Pointer. #Patch Set 11 : Moved num_priorities from Limits to PrioritizedDispatcher ctor' #Patch Set 12 : Fixes from try bots. #
Total comments: 30
Messages
Total messages: 20 (0 generated)
This is now ready for review. PTAL.
Working on fix for priority_queue.h on win. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:33: bool is_null() const { return iterator_ == ListIterator(); } This is not allowed in MSVC. Working on fix. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:41: return iterator_ == other.iterator_; Ditto, when the iterators are from different lists.
A bunch of fairly minor comments. Note that I've only skimmed HostResolverImpl. http://codereview.chromium.org/8965025/diff/19026/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/events_view.css (right): http://codereview.chromium.org/8965025/diff/19026/chrome/browser/resources/ne... chrome/browser/resources/net_internals/events_view.css:76: #events-view-source-list-tbody .source_HOST_RESOLVER_IMPL_PROC_JOB, nit: Just to be consistent with the ordering elsewhere, please put this below HOST_RESOLVER_IMPL_REQUEST. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:24: #include "base/memory/weak_ptr.h" Since SupportsWeakPtr is defined in this file, this should be included in host_resolver_impl.h rather than here. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:365: typedef base::Callback<void(int, int, const AddressList&)> Callback; nit: Mind adding a linebreak here? http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:370: : key_(key), nit: Know this was like this before, but this should be a 4 space indent. Looks like 3 to me. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.h File net/base/host_resolver_impl.h (right): http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:31: // that same host are made during the jobs lifetime, they are attached to the nit: job's lifetime http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:79: // within unresponsive_delay_ time. We keep attempting to resolve the host While you're here, could you surround the variable names in this paragraph with "|"s? http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:93: scoped_refptr<HostResolverProc> resolver_proc_; The style guide doesn't allow the terminating underscore in struct member names (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names) http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:112: // ownership of the |cache| pointer, and will free it during destructor. nit: "during destruction" or "in the destructor" http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:115: // will use (not counting potential duplicate attempts). Should clarify it's the number of active jobs, rather than the number of queued jobs, now that we immediately create jobs. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:130: void SetMaxQueuedJobs(size_t max_queued); Since this is only used in one test, could just go ahead and make it private and friend the test, unless you think we may eventually need it in tests outside of net/ http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:130: void SetMaxQueuedJobs(size_t max_queued); nit: Also, think either |max| or |max_queued_jobs| would be better - variable names should usually be nouns. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:220: // Otherwise, the previous caller will delete it. Previous caller? http://codereview.chromium.org/8965025/diff/19026/net/base/priority_dispatch.h File net/base/priority_dispatch.h (right): http://codereview.chromium.org/8965025/diff/19026/net/base/priority_dispatch.... net/base/priority_dispatch.h:37: // the dispatcher is not owned by the dispatcher but must outlive it. Use I think you mean must not outlive it. Or must not still be enqueued on the dispatcher's destruction, at least. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_dispatch.... net/base/priority_dispatch.h:48: }; nit: Please add a linebreak here. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:27: // when the queue is destroyed, the value is erased, or moved. nit: "when the queue is destroyed or the value is erased or moved" http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:34: RequestPriority priority() const { return priority_; } nit: const http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:45: friend class PriorityQueue; nit: Could you point a line break here? http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:49: RequestPriority priority_; Currently, this can be const. iterator_, too. My other comments will change that, but at least the iterator can be made kept const, I believe. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:65: void Erase(const Pointer& pointer) { It's kinda odd to take a const reference, and then make it invalid. I'd suggest taking a non-const pointer instead, even though yes, you don't actually modify it. Same goes for Move(). Think that, for debugging purposes, it would also be a good idea to modify |pointer| so we can have a DCHECK on re-use. Setting its priority to MAX_PRIORITY is probably the simplest way to do this. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:73: Pointer Move(const Pointer& pointer, RequestPriority priority) { I think "ChangePriority" or "Reprioritize" or "UpdatePriority" would be clearer than "Move". http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:82: Pointer First() { Believe this and the next two functions can all be const. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue_uni... File net/base/priority_queue_unittest.cc (right): http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue_uni... net/base/priority_queue_unittest.cc:33: EXPECT_TRUE(queue.Last().is_null()); Should check OldestLowest(), too, and make sure it's also null after deleting all the entries somewhere below. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue_uni... net/base/priority_queue_unittest.cc:56: for (size_t i = 0; i < kNumPriorities; ++i) { Just to make the test a little simpler and self-contained, you might want to divide this into 3 separate tests. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue_uni... net/base/priority_queue_unittest.cc:57: pointers[i] = queue.Insert(i, priorities[i]); You don't use pointers[i] in after populating them here or below. http://codereview.chromium.org/8965025/diff/21052/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/8965025/diff/21052/net/base/net_log_event_type... net/base/net_log_event_type_list.h:101: // This event is created when a a HostResolverImpl::Job is evicted from nit: Remove second "a" http://codereview.chromium.org/8965025/diff/21052/net/base/net_log_event_type... net/base/net_log_event_type_list.h:106: // This event is created when a a HostResolverImpl::Job is started by nit: Remove second "a" http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.cc File net/base/priority_dispatch.cc (right): http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.cc:22: Limits limits = {{0, 0, 0, 0, max_idle}}; You're relying on NUM_PRIORITIES being 5 here. Think it's best to remove that dependency, since it doesn't appear anywhere else in your CL, that I can see. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.cc:29: total+= reserved_slots[i]; nit: Space before the +=. Applies elsewhere in this file, too. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.cc:82: if (num_running_jobs_ < max_running_jobs_[priority]) { You're pretty much duplicating half of OnJobFinished here. Not a lot of code, but think it's best to go ahead and make it a function. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.cc:90: return queue_.Move(handle, priority); I think it might be a little simpler, and not all that much more inefficient to just get rid of Move, and use Erase/Add instead. The only downside is you'd need to check at a higher level if the priority is unchanged. If you prefer it as it is, that's fine, just a suggestion. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.h File net/base/priority_dispatch.h (right): http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:20: class NET_EXPORT_PRIVATE PriorityDispatch { I think "PrioritizedDispatcher" would be a little clearer, or "PriorityBasedDispatcher", or some such. "Dispatch" is a verb rather than a noun, and it doesn't actually dispatch priorities. "PriorityQueueDispatcher" also works, though perhaps it's a bit less accurate. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:28: struct NET_EXPORT_PRIVATE Limits { I think this class would be simpler to used to you created with the total number of allowed sockets. Then use reserved slots to indicate how many of those sockets should be reserved at each level or higher. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:30: static Limits MakeAny(size_t max_idle); nit: Think "max_idle_jobs" is a bit clearer. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:38: // Cancel to remove job from queue. I think you mean "must not outlive it", or at least "must not still be queued when the dispatcher is destroyed". http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:48: }; nit: Please add a blank line here. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:62: // Configures |max_queued|. Allowed only when queue is empty. "Allowed only" -> "May be called only" http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:70: // Removes the job at |handle| from the queue. "the Job with |handle|". Should also mention it invalidates the Handle. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:74: // |priority| and returns the updated handle or null-handle if the job is nit: "handle, or a null handle if it starts the job" http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:85: // priority. Not quite accurate. It's the maximum number of running jobs that can result from starting a new job at each priority (max for starting + 1) http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch_... File net/base/priority_dispatch_unittest.cc (right): http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch_... net/base/priority_dispatch_unittest.cc:21: // evicted, and '.' when finished. nit: log_ http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch_... net/base/priority_dispatch_unittest.cc:21: // evicted, and '.' when finished. I think a bit more of an explanation is in order. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch_... net/base/priority_dispatch_unittest.cc:27: // win does not accept EXPECT_EQ(this, ...) so wrap it up. nit: "Windows does not" http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch_... net/base/priority_dispatch_unittest.cc:123: friend class TestJob; nit: Don't believe this is needed. Inner classes can access all of their outer class's variables, can't they?
Thanks for the review so far. I've only added comments to where some explanation
was needed. "Quick-Done" everywhere else (will fix when you're done with first
pass).
I am contemplating changing PriorityQueue+Pointer to use LinkNode+LinkedList
from base/linked_list.h as follows:
template<typename T>
class Node : public base::LinkNode<Node> {
// accessors here
private:
T value_;
int priority_;
}
This would introduce redundant storage of |priority_| but this way the Node* can
serve as pointer and will remain valid through Update/Erase (only destruction of
the Node would invalidate it).
Thoughts?
http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.h
File net/base/host_resolver_impl.h (right):
http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl...
net/base/host_resolver_impl.h:220: // Otherwise, the previous caller will delete
it.
On 2011/12/21 16:22:58, Matt Menke wrote:
> Previous caller?
Whoever was the first to call RemoveJob and receive true.
http://codereview.chromium.org/8965025/diff/19026/net/base/priority_dispatch.h
File net/base/priority_dispatch.h (right):
http://codereview.chromium.org/8965025/diff/19026/net/base/priority_dispatch....
net/base/priority_dispatch.h:37: // the dispatcher is not owned by the
dispatcher but must outlive it. Use
On 2011/12/21 16:22:58, Matt Menke wrote:
> I think you mean must not outlive it. Or must not still be enqueued on the
> dispatcher's destruction, at least.
I wasn't clear: the dispatcher expects the Job to be available for call to
Start/OnEvicted as long as the Job is queued in the dispatcher. After the call
to Job::Start/OnEvicted, or after the dispatcher is destroyed, the Job can go
away.
http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h
File net/base/priority_queue.h (right):
http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n...
net/base/priority_queue.h:49: RequestPriority priority_;
On 2011/12/21 16:22:58, Matt Menke wrote:
> Currently, this can be const. iterator_, too. My other comments will change
> that, but at least the iterator can be made kept const, I believe.
If this is const, then the default operator= would not work.
http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n...
net/base/priority_queue.h:65: void Erase(const Pointer& pointer) {
On 2011/12/21 16:22:58, Matt Menke wrote:
> It's kinda odd to take a const reference, and then make it invalid. I'd
suggest
> taking a non-const pointer instead, even though yes, you don't actually modify
> it. Same goes for Move().
>
> Think that, for debugging purposes, it would also be a good idea to modify
> |pointer| so we can have a DCHECK on re-use. Setting its priority to
> MAX_PRIORITY is probably the simplest way to do this.
Agreed that invalidating |pointer| would make sense. However, an API that takes
Pointer* would forbid |queue.Erase(queue.Front())|
http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.h
File net/base/priority_dispatch.h (right):
http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch....
net/base/priority_dispatch.h:28: struct NET_EXPORT_PRIVATE Limits {
On 2011/12/21 16:22:58, Matt Menke wrote:
> I think this class would be simpler to used to you created with the total
number
> of allowed sockets. Then use reserved slots to indicate how many of those
> sockets should be reserved at each level or higher.
Ok, although this will require a DCHECK that:
sum(reserved_slots) <= total_available_slots
Sorry, meant to get back to you with a full review later today. Got a bit sidetracked. I'll try to get back to you tonight, if not then, tomorrow am. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:65: void Erase(const Pointer& pointer) { On 2011/12/21 22:19:34, szym wrote: > On 2011/12/21 16:22:58, Matt Menke wrote: > > It's kinda odd to take a const reference, and then make it invalid. I'd > suggest > > taking a non-const pointer instead, even though yes, you don't actually modify > > it. Same goes for Move(). > > > > Think that, for debugging purposes, it would also be a good idea to modify > > |pointer| so we can have a DCHECK on re-use. Setting its priority to > > MAX_PRIORITY is probably the simplest way to do this. > > Agreed that invalidating |pointer| would make sense. However, an API that takes > Pointer* would forbid |queue.Erase(queue.Front())| I believe that |queue.Erase(&queue.Front())| should still work.
http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:270: class HostResolverImpl::Request { Think we should add a comment here that these are owned by the Job, and exist until the Job is destroyed, even when cancelled. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:363: : public base::RefCountedThreadSafe<HostResolverImpl::ProcJob> { I think using the names "Job" and "ProcJob" are a little confusing. As it is, when one sees "job", in this file, it's not clear if it means a Job or a ProcJob. I suggest removing the word "Job" entirely from one name or the other. ProcTask? SyncResolverTask? GetAddrInfoTask? I'm open to other ideas. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:832: explicit PriorityTracker(RequestPriority initial) Don't think there's any reason to take a RequestPriority as an argument, as we're going to have to be calling Add immediately anyways. Think we should just start out at IDLE, save some unnecessary argument passing. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:916: net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, NULL); Think we should log the error here, if any. Code would be: net_log_.EndEventWithNetError(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, net_error_); If net_error_ is non-negative, no error code will be logged. While it will be logged elsewhere, too, logging it here will make the Job have a red background on about:net-internals, which can be useful. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:958: "source_dependency", req->request_net_log().source()))); May be a little simpler to just always add the resulting priority to this event. Up to you. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:999: if (!net_log_.net_log()) Since this generally only happens during testing, don't think it's needed. All the logging functions check for this, anyways. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1045: if (net_log_.net_log()) { nit: Again, don't think this is needed. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1046: // Log end of queue. You don't actually seem to be logging anything here, just doing a DCHECK. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1066: // call OnProcJobComplete. nit: To be a little clearer, I think you should add something like "on synchronous failure" or some such. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1091: resolver_->OnJobFinished(this, addrlist); This is only called in response to OnProcJobComplete, right? Seems to make more sense to call it OnJobComplete, and call it from there, rather than checking against the two other failure cases here. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1125: // Tracks the highest priority across requests_. nit: |requests| http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1504: for (JobMap::iterator it = jobs_.begin(); it != jobs_.end(); ) { Looks to me like some of this complexity could be avoided if you just deleted jobs that haven't started yet here, too. Then you could also make RemoveJob delete the job being removed too, and reduce the complexity of CompleteRequests as well. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.h File net/base/host_resolver_impl.h (right): http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:224: void CancelAllJobs(); Mind renaming this to Shutdown() or ShutdownAllJobs()? Think that makes the behavior a little clearer than the current name.
http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:363: : public base::RefCountedThreadSafe<HostResolverImpl::ProcJob> { On 2011/12/22 16:18:49, Matt Menke wrote: > I think using the names "Job" and "ProcJob" are a little confusing. As it is, > when one sees "job", in this file, it's not clear if it means a Job or a > ProcJob. I suggest removing the word "Job" entirely from one name or the other. > ProcTask? SyncResolverTask? GetAddrInfoTask? I'm open to other ideas. Alternatively, could add a prefix to Job.
Thanks for the review. A couple questions below. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:832: explicit PriorityTracker(RequestPriority initial) On 2011/12/22 16:18:49, Matt Menke wrote: > Don't think there's any reason to take a RequestPriority as an argument, as > we're going to have to be calling Add immediately anyways. Think we should just > start out at IDLE, save some unnecessary argument passing. Not quite immediately. The Job is added to the dispatcher before the Request is Added to it (in fact, before the Request is even created). This way if the Job is evicted on Add we know not to run the callback. Alternatively, (1) we could put a flag in the Request as "do not call me back yet, because I'm in Resolve", or (2) swap in a temporary callback that stores the result to be later used once we return to Resolve. Any preference for one of the three: (current), (1), or (2)? http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:999: if (!net_log_.net_log()) On 2011/12/22 16:18:49, Matt Menke wrote: > Since this generally only happens during testing, don't think it's needed. All > the logging functions check for this, anyways. I don't think cancellations will happen only during testing, but I agree this is a pre-optimization. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1046: // Log end of queue. On 2011/12/22 16:18:49, Matt Menke wrote: > You don't actually seem to be logging anything here, just doing a DCHECK. Oops. Leftovers. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1091: resolver_->OnJobFinished(this, addrlist); On 2011/12/22 16:18:49, Matt Menke wrote: > This is only called in response to OnProcJobComplete, right? Seems to make more > sense to call it OnJobComplete, and call it from there, rather than checking > against the two other failure cases here. It makes sense for me to move the !was_evicted check to OnJobFinished, but we'll still need to check for ERR_ABORTED. The trickiness comes in the next two lines. Essentially, CompleteRequests needs to hold the job in |self_deleter| because otherwise it might be gone when req->OnComplete returns. (The Job could be Aborted or the resolver could be destroyed.) ERR_ABORTED needs to be checked, because AbortAllInProgressJobs iterates through |jobs_| and calling RemoveJob would invalidate the iterator. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1504: for (JobMap::iterator it = jobs_.begin(); it != jobs_.end(); ) { On 2011/12/22 16:18:49, Matt Menke wrote: > Looks to me like some of this complexity could be avoided if you just deleted > jobs that haven't started yet here, too. Then you could also make RemoveJob > delete the job being removed too, and reduce the complexity of CompleteRequests > as well. True, although this would change the current behavior which only aborts running jobs and the pending requests stick around. Would that change be ok?
http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:832: explicit PriorityTracker(RequestPriority initial) On 2011/12/22 16:56:57, szym wrote: > On 2011/12/22 16:18:49, Matt Menke wrote: > > Don't think there's any reason to take a RequestPriority as an argument, as > > we're going to have to be calling Add immediately anyways. Think we should > just > > start out at IDLE, save some unnecessary argument passing. > > Not quite immediately. > > The Job is added to the dispatcher before the Request is Added to it (in fact, > before the Request is even created). This way if the Job is evicted on Add we > know not to run the callback. > > Alternatively, (1) we could put a flag in the Request as "do not call me back > yet, because I'm in Resolve", or (2) swap in a temporary callback that stores > the result to be later used once we return to Resolve. > > Any preference for one of the three: (current), (1), or (2)? Ah, good point. Think it's fine as-is, then. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:997: proc_job_ = NULL; nit: I think calling release would be a little clearer. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:999: if (!net_log_.net_log()) On 2011/12/22 16:56:57, szym wrote: > On 2011/12/22 16:18:49, Matt Menke wrote: > > Since this generally only happens during testing, don't think it's needed. > All > > the logging functions check for this, anyways. > > I don't think cancellations will happen only during testing, but I agree this is > a pre-optimization. Sorry, I mean net_log_ will probably only be null during testing. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1091: resolver_->OnJobFinished(this, addrlist); On 2011/12/22 16:56:57, szym wrote: > On 2011/12/22 16:18:49, Matt Menke wrote: > > This is only called in response to OnProcJobComplete, right? Seems to make > more > > sense to call it OnJobComplete, and call it from there, rather than checking > > against the two other failure cases here. > > It makes sense for me to move the !was_evicted check to OnJobFinished, but we'll > still need to check for ERR_ABORTED. > > The trickiness comes in the next two lines. Essentially, CompleteRequests needs > to hold the job in |self_deleter| because otherwise it might be gone when > req->OnComplete returns. (The Job could be Aborted or the resolver could be > destroyed.) > > ERR_ABORTED needs to be checked, because AbortAllInProgressJobs iterates through > |jobs_| and calling RemoveJob would invalidate the iterator. Hmm...Actually, don't we need to call OnJobFinished on ERR_ABORTED when is_running() is true? A little more discussion on the deletion stuff below. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1504: for (JobMap::iterator it = jobs_.begin(); it != jobs_.end(); ) { On 2011/12/22 16:56:57, szym wrote: > On 2011/12/22 16:18:49, Matt Menke wrote: > > Looks to me like some of this complexity could be avoided if you just deleted > > jobs that haven't started yet here, too. Then you could also make RemoveJob > > delete the job being removed too, and reduce the complexity of > CompleteRequests > > as well. > > True, although this would change the current behavior which only aborts running > jobs and the pending requests stick around. Would that change be ok? Hmm... Good point. I'm really not fond of the current dependencies on how cleanup in here and in CompleteRequests works. I don't think changing behavior would be a huge deal, since we're aborting 100 requests anyways. However, we could keep the current behavior by incrementing |it| first, right? Then job->Abort could delete the job, too. I'm open to other workarounds, just think it's best for code not to delete stuff or not based on what's 2 functions up on the callstack, if we can avoid it.
http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:997: proc_job_ = NULL; On 2011/12/22 17:34:30, Matt Menke wrote: > nit: I think calling release would be a little clearer. Oops...Nevermind. Forgot that release with scoped_reftrs doesn't release the reference.
Ready for another pass. http://codereview.chromium.org/8965025/diff/19026/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/events_view.css (right): http://codereview.chromium.org/8965025/diff/19026/chrome/browser/resources/ne... chrome/browser/resources/net_internals/events_view.css:76: #events-view-source-list-tbody .source_HOST_RESOLVER_IMPL_PROC_JOB, On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Just to be consistent with the ordering elsewhere, please put this below > HOST_RESOLVER_IMPL_REQUEST. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:24: #include "base/memory/weak_ptr.h" On 2011/12/21 16:22:58, Matt Menke wrote: > Since SupportsWeakPtr is defined in this file, this should be included in > host_resolver_impl.h rather than here. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:270: class HostResolverImpl::Request { On 2011/12/22 16:18:49, Matt Menke wrote: > Think we should add a comment here that these are owned by the Job, and exist > until the Job is destroyed, even when cancelled. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:363: : public base::RefCountedThreadSafe<HostResolverImpl::ProcJob> { On 2011/12/22 16:18:49, Matt Menke wrote: > I think using the names "Job" and "ProcJob" are a little confusing. As it is, > when one sees "job", in this file, it's not clear if it means a Job or a > ProcJob. I suggest removing the word "Job" entirely from one name or the other. > ProcTask? SyncResolverTask? GetAddrInfoTask? I'm open to other ideas. ProcTask it is. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:365: typedef base::Callback<void(int, int, const AddressList&)> Callback; On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Mind adding a linebreak here? Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:370: : key_(key), On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Know this was like this before, but this should be a 4 space indent. > Looks like 3 to me. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:832: explicit PriorityTracker(RequestPriority initial) A new work-around in place using PrioritizedDispatcher::WouldEvict. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:916: net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, NULL); On 2011/12/22 16:18:49, Matt Menke wrote: > Think we should log the error here, if any. Code would be: > > net_log_.EndEventWithNetError(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, net_error_); > > If net_error_ is non-negative, no error code will be logged. While it will be > logged elsewhere, too, logging it here will make the Job have a red background > on about:net-internals, which can be useful. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:958: "source_dependency", req->request_net_log().source()))); On 2011/12/22 16:18:49, Matt Menke wrote: > May be a little simpler to just always add the resulting priority to this event. > Up to you. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1066: // call OnProcJobComplete. On 2011/12/22 16:18:49, Matt Menke wrote: > nit: To be a little clearer, I think you should add something like "on > synchronous failure" or some such. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1091: resolver_->OnJobFinished(this, addrlist); That's correct, although that was already done in the body of AbortAllInProgressJobs. In the new patch Job::Abort deletes the Job so OnJobFinished is called except when evicted. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1125: // Tracks the highest priority across requests_. On 2011/12/22 16:18:49, Matt Menke wrote: > nit: |requests| Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1504: for (JobMap::iterator it = jobs_.begin(); it != jobs_.end(); ) { Done. Job::Abort deletes the job. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl.h File net/base/host_resolver_impl.h (right): http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:31: // that same host are made during the jobs lifetime, they are attached to the On 2011/12/21 16:22:58, Matt Menke wrote: > nit: job's lifetime Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:79: // within unresponsive_delay_ time. We keep attempting to resolve the host On 2011/12/21 16:22:58, Matt Menke wrote: > While you're here, could you surround the variable names in this paragraph with > "|"s? Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:93: scoped_refptr<HostResolverProc> resolver_proc_; On 2011/12/21 16:22:58, Matt Menke wrote: > The style guide doesn't allow the terminating underscore in struct member names > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names) Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:112: // ownership of the |cache| pointer, and will free it during destructor. On 2011/12/21 16:22:58, Matt Menke wrote: > nit: "during destruction" or "in the destructor" Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:115: // will use (not counting potential duplicate attempts). On 2011/12/21 16:22:58, Matt Menke wrote: > Should clarify it's the number of active jobs, rather than the number of queued > jobs, now that we immediately create jobs. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:130: void SetMaxQueuedJobs(size_t max_queued); On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Also, think either |max| or |max_queued_jobs| would be better - variable > names should usually be nouns. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:220: // Otherwise, the previous caller will delete it. This part of the logic removed. http://codereview.chromium.org/8965025/diff/19026/net/base/host_resolver_impl... net/base/host_resolver_impl.h:224: void CancelAllJobs(); Removed. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_dispatch.h File net/base/priority_dispatch.h (right): http://codereview.chromium.org/8965025/diff/19026/net/base/priority_dispatch.... net/base/priority_dispatch.h:48: }; On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Please add a linebreak here. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:27: // when the queue is destroyed, the value is erased, or moved. On 2011/12/21 16:22:58, Matt Menke wrote: > nit: "when the queue is destroyed or the value is erased or moved" Done. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:45: friend class PriorityQueue; On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Could you point a line break here? Done. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:65: void Erase(const Pointer& pointer) { > I believe that |queue.Erase(&queue.Front())| should still work. Unfortunately, can't take address of a temporary. I left it unchanged. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:73: Pointer Move(const Pointer& pointer, RequestPriority priority) { On 2011/12/21 16:22:58, Matt Menke wrote: > I think "ChangePriority" or "Reprioritize" or "UpdatePriority" would be clearer > than "Move". Removed completely. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue.h#n... net/base/priority_queue.h:82: Pointer First() { On 2011/12/21 16:22:58, Matt Menke wrote: > Believe this and the next two functions can all be const. Unfortunately, then List would have be const and then we could only pass const_iterator to Pointer. This would make Pointer useless for Erase. This problem will go away in C++0x: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf Until then, I see two ways to resolve: A) Create Pointer vs. ConstPointer and two different version of each First/Last/OldestLowest. B) Move to base/linked_list.h. Although that is exploiting the fact that base::LinkedList can be const while other parts of it are modified. For now, I left it as is. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue_uni... File net/base/priority_queue_unittest.cc (right): http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue_uni... net/base/priority_queue_unittest.cc:33: EXPECT_TRUE(queue.Last().is_null()); On 2011/12/21 16:22:58, Matt Menke wrote: > Should check OldestLowest(), too, and make sure it's also null after deleting > all the entries somewhere below. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue_uni... net/base/priority_queue_unittest.cc:56: for (size_t i = 0; i < kNumPriorities; ++i) { On 2011/12/21 16:22:58, Matt Menke wrote: > Just to make the test a little simpler and self-contained, you might want to > divide this into 3 separate tests. Done. http://codereview.chromium.org/8965025/diff/19026/net/base/priority_queue_uni... net/base/priority_queue_unittest.cc:57: pointers[i] = queue.Insert(i, priorities[i]); On 2011/12/21 16:22:58, Matt Menke wrote: > You don't use pointers[i] in after populating them here or below. Done. http://codereview.chromium.org/8965025/diff/21052/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/8965025/diff/21052/net/base/net_log_event_type... net/base/net_log_event_type_list.h:101: // This event is created when a a HostResolverImpl::Job is evicted from On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Remove second "a" Done. http://codereview.chromium.org/8965025/diff/21052/net/base/net_log_event_type... net/base/net_log_event_type_list.h:106: // This event is created when a a HostResolverImpl::Job is started by On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Remove second "a" Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.cc File net/base/priority_dispatch.cc (right): http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.cc:22: Limits limits = {{0, 0, 0, 0, max_idle}}; On 2011/12/21 16:22:58, Matt Menke wrote: > You're relying on NUM_PRIORITIES being 5 here. Think it's best to remove that > dependency, since it doesn't appear anywhere else in your CL, that I can see. Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.cc:29: total+= reserved_slots[i]; On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Space before the +=. Applies elsewhere in this file, too. Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.cc:82: if (num_running_jobs_ < max_running_jobs_[priority]) { On 2011/12/21 16:22:58, Matt Menke wrote: > You're pretty much duplicating half of OnJobFinished here. Not a lot of code, > but think it's best to go ahead and make it a function. Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.cc:90: return queue_.Move(handle, priority); On 2011/12/21 16:22:58, Matt Menke wrote: > I think it might be a little simpler, and not all that much more inefficient to > just get rid of Move, and use Erase/Add instead. The only downside is you'd > need to check at a higher level if the priority is unchanged. > > If you prefer it as it is, that's fine, just a suggestion. Agreed. "Move" would make more sense if it preserved insertion order. Right now it's only saving one heap alloc and free for the std::list node. Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.h File net/base/priority_dispatch.h (right): http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:20: class NET_EXPORT_PRIVATE PriorityDispatch { On 2011/12/21 16:22:58, Matt Menke wrote: > I think "PrioritizedDispatcher" would be a little clearer, or > "PriorityBasedDispatcher", or some such. "Dispatch" is a verb rather than a > noun, and it doesn't actually dispatch priorities. "PriorityQueueDispatcher" > also works, though perhaps it's a bit less accurate. Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:20: class NET_EXPORT_PRIVATE PriorityDispatch { On 2011/12/21 16:22:58, Matt Menke wrote: > I think "PrioritizedDispatcher" would be a little clearer, or > "PriorityBasedDispatcher", or some such. "Dispatch" is a verb rather than a > noun, and it doesn't actually dispatch priorities. "PriorityQueueDispatcher" > also works, though perhaps it's a bit less accurate. Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:28: struct NET_EXPORT_PRIVATE Limits { On 2011/12/21 22:19:34, szym wrote: > On 2011/12/21 16:22:58, Matt Menke wrote: > > I think this class would be simpler to used to you created with the total > number > > of allowed sockets. Then use reserved slots to indicate how many of those > > sockets should be reserved at each level or higher. > > Ok, although this will require a DCHECK that: > sum(reserved_slots) <= total_available_slots Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:38: // Cancel to remove job from queue. On 2011/12/21 16:22:58, Matt Menke wrote: > I think you mean "must not outlive it", or at least "must not still be queued > when the dispatcher is destroyed". Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:48: }; On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Please add a blank line here. Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:62: // Configures |max_queued|. Allowed only when queue is empty. On 2011/12/21 16:22:58, Matt Menke wrote: > "Allowed only" -> "May be called only" Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:70: // Removes the job at |handle| from the queue. On 2011/12/21 16:22:58, Matt Menke wrote: > "the Job with |handle|". > > Should also mention it invalidates the Handle. Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:74: // |priority| and returns the updated handle or null-handle if the job is On 2011/12/21 16:22:58, Matt Menke wrote: > nit: "handle, or a null handle if it starts the job" Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch.... net/base/priority_dispatch.h:85: // priority. On 2011/12/21 16:22:58, Matt Menke wrote: > Not quite accurate. > > It's the maximum number of running jobs that can result from starting a new job > at each priority (max for starting + 1) Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch_... File net/base/priority_dispatch_unittest.cc (right): http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch_... net/base/priority_dispatch_unittest.cc:21: // evicted, and '.' when finished. On 2011/12/21 16:22:58, Matt Menke wrote: > I think a bit more of an explanation is in order. Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch_... net/base/priority_dispatch_unittest.cc:27: // win does not accept EXPECT_EQ(this, ...) so wrap it up. On 2011/12/21 16:22:58, Matt Menke wrote: > nit: "Windows does not" Done. http://codereview.chromium.org/8965025/diff/21052/net/base/priority_dispatch_... net/base/priority_dispatch_unittest.cc:123: friend class TestJob; On 2011/12/21 16:22:58, Matt Menke wrote: > nit: Don't believe this is needed. Inner classes can access all of their outer > class's variables, can't they? Done.
I probably won't be able to do a review until this weekend. Was planning to finish the review promptly, but I have a nasty cold, and don't think I'd do an adequate job (And don't really feel much like it, either).
I didn't take a significant look at the HostResolverImpl's usage of the PrioritizedDispatcher. I'm concerned about the complexity of the valid lifetime of Handles/Pointers. http://codereview.chromium.org/8965025/diff/20044/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8965025/diff/20044/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1251: if (dispatcher_.WouldEvict(info.priority())) { Why can't this just call Add and see if the returned handle is_null. http://codereview.chromium.org/8965025/diff/20044/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/8965025/diff/20044/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:32: size_t reserved_slots[NUM_PRIORITIES]; Like PriorityQueue, it feels like this should be decoupled from NUM_PRIORITIES. Perhaps have a vector<size_t> or something similar. http://codereview.chromium.org/8965025/diff/20044/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:64: // Configures |max_queued|. May be called only when queue is empty. Why isn't this part of Limits? http://codereview.chromium.org/8965025/diff/20044/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:68: bool WouldEvict(RequestPriority priority) const; From the one caller I saw, it looks like this method could be removed and Add could be called with a check for is_null - assuming that a different approach is taking for Handle if the job is immediately started. http://codereview.chromium.org/8965025/diff/20044/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:76: void Cancel(const Handle& handle); I think this needs to be a bit more defensive about handling invalidated Handle's. For example, lets say Add is called and the Job is unable to start right away so it's queued up and a non-null Handle is returned. At some later point, the Job is started because another completes. While this happens, Cancel is called. Now arguably this shouldn't happen. Start is called on the Job so Cancel should no longer be called on the Handle. But that adds a lot of tight coupling between the Job itself and the owner of the job lifetime. It would be nicer if Cancel could handle invalid handles as a no-op. http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:17: // A simple priority queue. The order of values is by RequestPriority and then It would be nice if this weren't coupled with RequestPriority/NUM_PRIORITIES so it could be used in more situations. Either make the number of priorities a constructor argument, or possibly a non-type template parameter (although I am worried about code duplication in that case). http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:18: // FIFO. The queue supports changing the priority of its elements. How do you change the priority of the elements? I can't see anything about that, other than doing an Erase followed by an Insert as a caller. http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:19: // All operations are O(1) time (assuming constant NUM_PRIORITIES). You should document why and where this is needed instead of the STL's priority_queue. http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:21: class PriorityQueue { This should probably derive from NonThreadSafe or have some other assertions to validate that it's always being accessed from the same thread. http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:27: typedef typename std::list<T>::iterator ListIterator; ListIterator could probably be typedef'd within Pointer instead of PriorityQueue. http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:68: void Erase(const Pointer& pointer) { Should this be a member on Pointer? Also, should this take a non-const Pointer since it will ultimately invalidate the pointer? http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:68: void Erase(const Pointer& pointer) { It feels like this class could benefit with some debug invariant checks - like making sure that size_ > 0 before doing this. http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:85: for (size_t i = 0; i < NUM_PRIORITIES; ++i) { Why not do a i = NUM_PRIORITIES; i > 0; --i? Need to be more careful with unsigned, but this should be ok and more obvious.
Thanks for the comments. I'm leaning more towards LinkedList given the preference to safely invalidate pointers. (std::list provides no portable way to determine that an iterator is invalid.) Regarding queuing limits and eviction: I'm concerned if PrioritizedDispatcher should enforce the queue size at all. Ultimately, we'll have two dispatchers in HostResolverImpl: one dispatching Jobs and the other dispatching ProcTasks. If the dispatchers enforce queuing limits, then it seems to me they should be hierarchical rather than serial and the inner ProcTask dispatcher should have no queue size limit. Otherwise we would have to deal with the situation that a Job which already performed a DNS request has to be evicted from the ProcTask dispatcher. But this means that queue size limit is enforced at the HostResolverImpl level. Therefore, I'm considering removing the eviction logic from PrioritizedDispatcher. This would also allow us to use the two dispatchers in series or in parallel if necessary (for example, for DnsTask and ProcTask). http://codereview.chromium.org/8965025/diff/20044/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8965025/diff/20044/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1251: if (dispatcher_.WouldEvict(info.priority())) { On 2012/01/03 19:39:09, cbentzel wrote: > Why can't this just call Add and see if the returned handle is_null. Left comments in the .h file. http://codereview.chromium.org/8965025/diff/20044/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/8965025/diff/20044/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:64: // Configures |max_queued|. May be called only when queue is empty. On 2012/01/03 19:39:09, cbentzel wrote: > Why isn't this part of Limits? It makes sense, but note my concern about enforcing queue size at the dispatcher level. http://codereview.chromium.org/8965025/diff/20044/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:68: bool WouldEvict(RequestPriority priority) const; On 2012/01/03 19:39:09, cbentzel wrote: > From the one caller I saw, it looks like this method could be removed and Add > could be called with a check for is_null - assuming that a different approach is > taking for Handle if the job is immediately started. We must not call OnEvicted on the Job that is being Added. Two reasons I prefer WouldEvict: - No need to even create the Job if it would be evicted. - No need for Job to provide its own mechanism to distinguish between eviction and start on Add. (Although that makes the code checking if the evicted Job is being added dead.) One reason I would prefer just Add: - Add needs to redo what WouldEvict already does. http://codereview.chromium.org/8965025/diff/20044/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:76: void Cancel(const Handle& handle); On 2012/01/03 19:39:09, cbentzel wrote: > I think this needs to be a bit more defensive about handling invalidated > Handle's. > > For example, lets say Add is called and the Job is unable to start right away so > it's queued up and a non-null Handle is returned. > > At some later point, the Job is started because another completes. While this > happens, Cancel is called. > > Now arguably this shouldn't happen. Start is called on the Job so Cancel should > no longer be called on the Handle. > > But that adds a lot of tight coupling between the Job itself and the owner of > the job lifetime. It would be nicer if Cancel could handle invalid handles as a > no-op. Agreed, although I don't really have a better contract than: "please invalidate your handle after either OnStart or OnEvict is called, and after you called Update or Cancel." The issue with PriorityQueue using std::list is that AFAIK there's no (portable) way to determine that an iterator is invalid. If we switched to LinkedList then the Handle could essentially be a pointer to a LinkNode, and so it would remain usable until the Job is destroyed. It could then store a pointer to the PriorityQueue to check if it's an element of it or not. The downside of using LinkedList is that then we need to add stuff to the pure-virtual Job interface making it impure. That said, I'm leaning towards that solution. http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:17: // A simple priority queue. The order of values is by RequestPriority and then On 2012/01/03 19:39:09, cbentzel wrote: > It would be nice if this weren't coupled with RequestPriority/NUM_PRIORITIES so > it could be used in more situations. > > Either make the number of priorities a constructor argument, or possibly a > non-type template parameter (although I am worried about code duplication in > that case). Having NUM_PRIORITIES const allows the compiler to unroll some of the for loops, but I doubt that it actually matters much. One caveat if we want decouple from RequestPriority/NUM_PRIORITIES: the callers will need to explicitly cast Pointer/Handle::priority() from int to RequestPriority. http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:18: // FIFO. The queue supports changing the priority of its elements. On 2012/01/03 19:39:09, cbentzel wrote: > How do you change the priority of the elements? I can't see anything about that, > other than doing an Erase followed by an Insert as a caller. There used to be Update/Move which essentially Erased then Inserted (with slightly less work). Will update comment. http://codereview.chromium.org/8965025/diff/20044/net/base/priority_queue.h#n... net/base/priority_queue.h:68: void Erase(const Pointer& pointer) { On 2012/01/03 19:39:09, cbentzel wrote: > Should this be a member on Pointer? > > Also, should this take a non-const Pointer since it will ultimately invalidate > the pointer? Since First returns a temporary, to allow queue.Erase(queue.First()) we need either a const reference, or a value. If we moved to LinkedList then there might be no need for a temporary in First, although that would add some redundant storage to Job (priority, *queue). The benefit would be that all "pointers" to the same element would be invalidated at once.
I have updated the CL with three patches: 1) Moved eviction logic from PrioritizedDispatcher to HostResolverImpl. 2) Decoupled PriorityQueue and PrioritizedDispatcher from RequestPriority. The queue is oblivious to priority order (is 0 highest or lowest), but PrioritizedDispatcher assumes 0 is top priority. 3) Added id_ to PriorityQueue::Pointer to catch invalid pointers.
As I mentioned in person - I think it would be good to split this into two CLs now that you are happy with the overall approach. First CL is simply adding the PriorityQueue and PrioritizedDispatcher, second modifies HostResolverImpl to use this. http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.cc (right): http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.cc:17: for (size_t i = num_priorities; i < limits.reserved_slots.size(); ++i) { Why aren't reserved_slots.size() and num_priorities required to be the same? http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.cc:42: job->Start(); It looks like there are no re-entrancy problems if OnJobFinished is called within Start (both here, and inside of DispatchJob). Perhaps worth mentioning. http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.cc:98: DCHECK(job); This DCHECK isn't needed - you check it at Add time which is more appropriate. http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:24: class NET_EXPORT_PRIVATE PrioritizedDispatcher { Make it explicit that this is not thread safe. http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:45: class Job { I wonder if there should be an explicit "Finish()" method on Job which calls OnJobFinished() on the associated PrioritizedDispatcher. This would also require the base class to hold a pointer to the dispatcher. http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:73: // Removes the job with |handle| from the queue. Invalidates |handle|. This should probably have a stronger warning that the Job associated with |handle| needs to be queued but not started. http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:74: void Cancel(const Handle& handle); Should this return a Job* to be closer to the eviction method? http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:83: Handle Update(const Handle& handle, size_t priority); ChangePriority would be a clearer name. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:22: // Unlike the std::priority_queue, all operations are O(p) time for p priority This also allows erasing of elements in the queue, as well as getting the oldest items. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:30: class PriorityQueue { I think this should derive from NonThreadSafe or have some other asserts to show that it's always being accessed from the same thread. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:33: // when the queue is destroyed or the value is erased or moved. Nit: remove "or moved". There is no direct support for moving an item in the queue. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:41: size_t priority() const { return priority_; } I'd argue for just using uint32 rather than a size_t, as it has nothing to do with sizeof() or byte count, etc. It would probably be reasonable to also add a typedef uint32 Level; or something similar to prevent repetition of the size_t/uint32 throughout. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:43: #ifndef NDEBUG Nit: #if !defined(NDEBUG) is preferred over #ifndef See http://dev.chromium.org/developers/coding-style http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:60: typedef typename std::list<std::pair<size_t, T> >::iterator ListIterator; Could be PriorityQueue::List::iterator to prevent needing to duplicate the debug/non-debug version (I think). http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:84: PriorityQueue(size_t num_priorities) explicit constructor needed. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:101: return Pointer(priority, list.insert(list.end(), Nit: list.push_back would be clearer than list.insert(list.end() http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:126: if (!size()) return Pointer(); 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/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:127: for (size_t i = 0; i < lists_.size(); ++i) Please use braces in this for loop. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:160: for (size_t i = lists_.size(); i > 0; --i) To be pedantic, this should be ListVector::size_type, but the assumption that it's size_t is done throughout the code base. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:161: if (!lists_[i - 1].empty()) Nit: probably clearer to have a local variable index = i - 1; http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:169: for (size_t i = 0; i < lists_.size(); ++i) BUG: valid_ids_ needs to be cleared here as well. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:193: size_t size_; Nit: extra new line. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:194: Should copy-and-assignment be disallowed? I don't think so, and it looks like it will work even in the presence of valid_ids_. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue_uni... File net/base/priority_queue_unittest.cc (right): http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue_uni... net/base/priority_queue_unittest.cc:72: EXPECT_EQ(kMaxOrder[kNumElements - i - 1], queue.LastMin().value()); Clever reuse of kMaxOrder - but may be clearer to just explicitly list the order desired.
It makes sense to split this CL into two. I will do that. How do you feel about moving Cancel/ChangePriority/Finish inside the Job (making it impure) and perhaps using a WeakPtr<PrioritizedDispatcher> for lifetime safety? http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.cc (right): http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.cc:17: for (size_t i = num_priorities; i < limits.reserved_slots.size(); ++i) { On 2012/01/05 18:32:20, cbentzel wrote: > Why aren't reserved_slots.size() and num_priorities required to be the same? This would require callers of HostResolverImpl() to know NUM_PRIORITIES. http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.cc:42: job->Start(); On 2012/01/05 18:32:20, cbentzel wrote: > It looks like there are no re-entrancy problems if OnJobFinished is called > within Start (both here, and inside of DispatchJob). Perhaps worth mentioning. Already mentioned. http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... File net/base/prioritized_dispatcher.h (right): http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:45: class Job { On 2012/01/05 18:32:20, cbentzel wrote: > I wonder if there should be an explicit "Finish()" method on Job which calls > OnJobFinished() on the associated PrioritizedDispatcher. This would also require > the base class to hold a pointer to the dispatcher. In that case it also makes sense for the Job to keep its own Handle. We could move most of the interface to the Job as well: AddTo(dispatcher), Cancel(), ChangePriority(new_priority). This precludes a Job being dispatched by two dispatchers at the same time, but I don't think that would be particularly useful. One caveat: now the dispatcher has to out-live the Job, unless we use WeakPtr. http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispat... net/base/prioritized_dispatcher.h:74: void Cancel(const Handle& handle); On 2012/01/05 18:32:20, cbentzel wrote: > Should this return a Job* to be closer to the eviction method? handle.value() returns the job. The caller of EvictOldestLowest has no handle. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h File net/base/priority_queue.h (right): http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:101: return Pointer(priority, list.insert(list.end(), On 2012/01/05 18:32:20, cbentzel wrote: > Nit: list.push_back would be clearer than list.insert(list.end() list.push_back returns void. http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#n... net/base/priority_queue.h:194: On 2012/01/05 18:32:20, cbentzel wrote: > Should copy-and-assignment be disallowed? I don't think so, and it looks like it > will work even in the presence of valid_ids_. It can be allowed. But for safety Pointers need to have a reference to the queue (ifndef NDEBUG).
On Thu, Jan 5, 2012 at 2:00 PM, <szym@chromium.org> wrote: > It makes sense to split this CL into two. I will do that. How do you feel > about > moving Cancel/ChangePriority/Finish inside the Job (making it impure) and > perhaps using a WeakPtr<PrioritizedDispatcher> for lifetime safety? That seems pretty reasonable. > > > > http://codereview.chromium.**org/8965025/diff/36019/net/** > base/prioritized_dispatcher.cc<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.cc> > File net/base/prioritized_**dispatcher.cc (right): > > http://codereview.chromium.**org/8965025/diff/36019/net/** > base/prioritized_dispatcher.**cc#newcode17<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.cc#newcode17> > net/base/prioritized_**dispatcher.cc:17: for (size_t i = num_priorities; i > < limits.reserved_slots.size(); ++i) { > On 2012/01/05 18:32:20, cbentzel wrote: > >> Why aren't reserved_slots.size() and num_priorities required to be the >> > same? > > This would require callers of HostResolverImpl() to know NUM_PRIORITIES. > > > http://codereview.chromium.**org/8965025/diff/36019/net/** > base/prioritized_dispatcher.**cc#newcode42<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.cc#newcode42> > net/base/prioritized_**dispatcher.cc:42: job->Start(); > On 2012/01/05 18:32:20, cbentzel wrote: > >> It looks like there are no re-entrancy problems if OnJobFinished is >> > called > >> within Start (both here, and inside of DispatchJob). Perhaps worth >> > mentioning. > > Already mentioned. > > > http://codereview.chromium.**org/8965025/diff/36019/net/** > base/prioritized_dispatcher.h<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.h> > File net/base/prioritized_**dispatcher.h (right): > > http://codereview.chromium.**org/8965025/diff/36019/net/** > base/prioritized_dispatcher.h#**newcode45<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.h#newcode45> > net/base/prioritized_**dispatcher.h:45: class Job { > On 2012/01/05 18:32:20, cbentzel wrote: > >> I wonder if there should be an explicit "Finish()" method on Job which >> > calls > >> OnJobFinished() on the associated PrioritizedDispatcher. This would >> > also require > >> the base class to hold a pointer to the dispatcher. >> > > In that case it also makes sense for the Job to keep its own Handle. We > could move most of the interface to the Job as well: AddTo(dispatcher), > Cancel(), ChangePriority(new_priority). This precludes a Job being > dispatched by two dispatchers at the same time, but I don't think that > would be particularly useful. > > One caveat: now the dispatcher has to out-live the Job, unless we use > WeakPtr. > > > http://codereview.chromium.**org/8965025/diff/36019/net/** > base/prioritized_dispatcher.h#**newcode74<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.h#newcode74> > net/base/prioritized_**dispatcher.h:74: void Cancel(const Handle& handle); > On 2012/01/05 18:32:20, cbentzel wrote: > >> Should this return a Job* to be closer to the eviction method? >> > > handle.value() returns the job. The caller of EvictOldestLowest has no > handle. > > > http://codereview.chromium.**org/8965025/diff/36019/net/** > base/priority_queue.h<http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h> > File net/base/priority_queue.h (right): > > http://codereview.chromium.**org/8965025/diff/36019/net/** > base/priority_queue.h#**newcode101<http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#newcode101> > net/base/priority_queue.h:101: return Pointer(priority, > list.insert(list.end(), > On 2012/01/05 18:32:20, cbentzel wrote: > >> Nit: list.push_back would be clearer than list.insert(list.end() >> > > list.push_back returns void. > > http://codereview.chromium.**org/8965025/diff/36019/net/** > base/priority_queue.h#**newcode194<http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#newcode194> > net/base/priority_queue.h:194: > > On 2012/01/05 18:32:20, cbentzel wrote: > >> Should copy-and-assignment be disallowed? I don't think so, and it >> > looks like it > >> will work even in the presence of valid_ids_. >> > > It can be allowed. But for safety Pointers need to have a reference to > the queue (ifndef NDEBUG). > > http://codereview.chromium.**org/8965025/<http://codereview.chromium.org/8965... >
On Thu, Jan 5, 2012 at 2:15 PM, Chris Bentzel <cbentzel@chromium.org> wrote: > > > On Thu, Jan 5, 2012 at 2:00 PM, <szym@chromium.org> wrote: > >> It makes sense to split this CL into two. I will do that. How do you feel >> about >> moving Cancel/ChangePriority/Finish inside the Job (making it impure) and >> perhaps using a WeakPtr<PrioritizedDispatcher> for lifetime safety? > > > That seems pretty reasonable. > Actually, I'm less sure about putting Cancel and ChangePriority inside of Job as you'll need to map back to a handle object or other iterator. I do like the WeakPtr suggestion. > > >> >> >> >> http://codereview.chromium.**org/8965025/diff/36019/net/** >> base/prioritized_dispatcher.cc<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.cc> >> File net/base/prioritized_**dispatcher.cc (right): >> >> http://codereview.chromium.**org/8965025/diff/36019/net/** >> base/prioritized_dispatcher.**cc#newcode17<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.cc#newcode17> >> net/base/prioritized_**dispatcher.cc:17: for (size_t i = num_priorities; >> i >> < limits.reserved_slots.size(); ++i) { >> On 2012/01/05 18:32:20, cbentzel wrote: >> >>> Why aren't reserved_slots.size() and num_priorities required to be the >>> >> same? >> >> This would require callers of HostResolverImpl() to know NUM_PRIORITIES. >> >> >> http://codereview.chromium.**org/8965025/diff/36019/net/** >> base/prioritized_dispatcher.**cc#newcode42<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.cc#newcode42> >> net/base/prioritized_**dispatcher.cc:42: job->Start(); >> On 2012/01/05 18:32:20, cbentzel wrote: >> >>> It looks like there are no re-entrancy problems if OnJobFinished is >>> >> called >> >>> within Start (both here, and inside of DispatchJob). Perhaps worth >>> >> mentioning. >> >> Already mentioned. >> >> >> http://codereview.chromium.**org/8965025/diff/36019/net/** >> base/prioritized_dispatcher.h<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.h> >> File net/base/prioritized_**dispatcher.h (right): >> >> http://codereview.chromium.**org/8965025/diff/36019/net/** >> base/prioritized_dispatcher.h#**newcode45<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.h#newcode45> >> net/base/prioritized_**dispatcher.h:45: class Job { >> On 2012/01/05 18:32:20, cbentzel wrote: >> >>> I wonder if there should be an explicit "Finish()" method on Job which >>> >> calls >> >>> OnJobFinished() on the associated PrioritizedDispatcher. This would >>> >> also require >> >>> the base class to hold a pointer to the dispatcher. >>> >> >> In that case it also makes sense for the Job to keep its own Handle. We >> could move most of the interface to the Job as well: AddTo(dispatcher), >> Cancel(), ChangePriority(new_priority). This precludes a Job being >> dispatched by two dispatchers at the same time, but I don't think that >> would be particularly useful. >> >> One caveat: now the dispatcher has to out-live the Job, unless we use >> WeakPtr. >> >> >> http://codereview.chromium.**org/8965025/diff/36019/net/** >> base/prioritized_dispatcher.h#**newcode74<http://codereview.chromium.org/8965025/diff/36019/net/base/prioritized_dispatcher.h#newcode74> >> net/base/prioritized_**dispatcher.h:74: void Cancel(const Handle& >> handle); >> On 2012/01/05 18:32:20, cbentzel wrote: >> >>> Should this return a Job* to be closer to the eviction method? >>> >> >> handle.value() returns the job. The caller of EvictOldestLowest has no >> handle. >> >> >> http://codereview.chromium.**org/8965025/diff/36019/net/** >> base/priority_queue.h<http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h> >> File net/base/priority_queue.h (right): >> >> http://codereview.chromium.**org/8965025/diff/36019/net/** >> base/priority_queue.h#**newcode101<http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#newcode101> >> net/base/priority_queue.h:101: return Pointer(priority, >> list.insert(list.end(), >> On 2012/01/05 18:32:20, cbentzel wrote: >> >>> Nit: list.push_back would be clearer than list.insert(list.end() >>> >> >> list.push_back returns void. >> >> http://codereview.chromium.**org/8965025/diff/36019/net/** >> base/priority_queue.h#**newcode194<http://codereview.chromium.org/8965025/diff/36019/net/base/priority_queue.h#newcode194> >> net/base/priority_queue.h:194: >> >> On 2012/01/05 18:32:20, cbentzel wrote: >> >>> Should copy-and-assignment be disallowed? I don't think so, and it >>> >> looks like it >> >>> will work even in the presence of valid_ids_. >>> >> >> It can be allowed. But for safety Pointers need to have a reference to >> the queue (ifndef NDEBUG). >> >> http://codereview.chromium.**org/8965025/<http://codereview.chromium.org/8965... >> > >
On Thu, Jan 5, 2012 at 2:17 PM, Chris Bentzel <cbentzel@chromium.org> wrote: > > Actually, I'm less sure about putting Cancel and ChangePriority inside of > Job as you'll need to map back to a handle object or other iterator. I do > like the WeakPtr suggestion. > I was assuming the Job would hold a Handle to itself. In most cases, I expect the users of PrioritizedDispatcher to do that anyway (see HostResolverImpl::Job). This would also allow DispatchJob to invalidate that handle which should make the API safer. In fact, we could get make Handle private and just talk to the dispatcher via the Job interface. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
