Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9101011:
[net/dns] Refactoring of job dispatch in HostResolverImpl in preparation for DnsTransactionFactory. (Closed)
|
Created:
8 years, 11 months ago by szym Modified:
8 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, darin-cc_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[net/dns] Refactoring of job dispatch in HostResolverImpl in preparation for DnsTransactionFactory.
- Replaces JobPool with PrioritizedDispatcher.
- 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 DnsTransactionFactory and then before ProcTask.)
This is the second CL in series to merge AsyncHostResolver into
HostResolverImpl.
BUG=107880
TEST=waterfall
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120114
Patch Set 1 #Patch Set 2 : Fixed use of MutableSetPort. #
Total comments: 40
Patch Set 3 : Addressed review: introduced CallSystemHostResolverProc, moved statics to anon namespace, ...' #
Total comments: 31
Patch Set 4 : Responded to code review; added TODOs for things bumped to another CL. #
Total comments: 1
Patch Set 5 : Rebased to master. #Patch Set 6 : Fixes to NetLog issues. #Patch Set 7 : inc(Copyright Year) #Patch Set 8 : Fixed license header for the presubmit check.' #
Messages
Total messages: 24 (0 generated)
Moved from http://codereview.chromium.org/8965025/
Matt - I definitely defer to you on the NetLog stuff. It would also probably be worth getting Eric Roman to take a once-over on this. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (left): http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1214: // Build a key that identifies the request in the cache and in the Why did you get rid of this comment? You didn't in ::Resolve( http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:160: HostResolver* CreateNonCachingSystemHostResolver(size_t max_concurrent_resolves, This should share a helper function with CreateSystemHostResolver - lots of duplicate logic. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:424: const ProcTaskParams& params, Nit: line up arguments http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:438: if (!params_.resolver_proc) The logic for which proc to use is a bit confusing - partly residual from before. if a non-NULL proc is specified, use it. otherwise, use the default one at the time the ProcTask is created. otherwise, use SystemHostResolverProc on the worker thread. It would be a lot clearer if this always needed to be non-NULL, and convert SystemHostResolverProc over to be a HostResolverProc class. I don't think it needs to be addressed in this CL, but might be a good way to go. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:477: DCHECK(origin_loop_->BelongsToCurrentThread()); This looks incorrect. It will return true even if the resolution completes successfully and was never explicitly canceled, due to the reset at the end of OnLookupComplete. This will be bad for statistics gathering on re-issues. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:888: class PriorityTracker { I'm not complete opposed to this - but it seems like a linear scan across the request priorities would be fine and simpler, since I don't expect that count to get too large. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1014: PrioritizedDispatcher::Handle& handle() { non-const ref returns like these are uncommon (and perhaps disallowed). I'd much rather see a set_handle() http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1049: req->MarkAsCanceled(); What's the rationale for marking as canceled instead of removing the Request from the list and deleting it? A good amount of the PriorityTracker logic would get simpler in that case. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1061: void OnProcTaskComplete(int net_error, int os_error, Could this be private? http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1062: const AddressList& addrlist) { Nit: line up columns. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1115: base::Bind(&Job::OnProcTaskComplete, base::Unretained(this)), I wonder if the callback creation and bound method should be on the HostResolverImpl. It would minimize the Job->Resolver calls, and tend to keep them Resolver->Job. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1131: // new job with the same key in case one of the OnComplete callbacks decides I wonder if we could do this after iterating through the requests? I'd like to see fewer paths for Job completion than currently exist, and will try to think some more about how to make that happen. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1294: if (dispatcher_.num_queued_jobs() > max_queued_jobs_) { Do you want to check if job->handle().is_null() here? Potentially want to evict only if we added something to the queue. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1295: Job* evicted = static_cast<Job*>(dispatcher_.EvictOldestLowest()); Why do you need to do the static_cast here? If you kept as base class, would the evicted == job compare against the base job class? http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.h File net/base/host_resolver_impl.h (right): http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.h:225: // Map from cache key to a job. Perhaps HostCache::Key in comment.
http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:135: NetLog* net_log) { nit: Just to reduce code duplication, I'd suggest making this and CreateNonCachingSystemHostResolver call the same function internally. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:305: virtual Value* ToValue() const { nit: OVERRIDE http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:320: // Holds the data for a request that could not be complete synchronously. nit: "not be completed" http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:424: const ProcTaskParams& params, nit: indent +1 http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:888: class PriorityTracker { nit: Explicit only needed for single argument constructors. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:888: class PriorityTracker { I also wonder if this class is really necessary. We only allow 6 HTTP requests (And thus connect jobs) per domain. With DNS prefetch, we may end up with more than 6, so shouldn't really matter. Suppose this class is small enough that getting rid of it wouldn't really simplify things much. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1076: // Aborts and destroys the job. nit: Think it's worth mentioning that it completes active requests. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1141: it != requests_.end(); ++it) { nit: Fix indent. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1155: // callback. If it was, we could continue, but we choose to bail. Why do we need to allow callbacks to be able to delete the HostResolverImpl, anyways? http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1240: // requests, which will also be canceled. No longer accurate. We now delete the jobs, which automatically cancel themselves. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1493: NetLog::TYPE_HOST_RESOLVER_IMPL_REQUEST, Random comment: Wonder if it would be worth considering not logging the requests. Suppose we'd lose speculative requests, particularly cache hits if we did that, so maybe not a great idea. Wouldn't belong in this CL, regardless. Suppose could log fake Job entries instead of Request entries instead. Maybe be better off just keeping it as it is. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1499: void HostResolverImpl::LogFinishRequest(const BoundNetLog& source_net_log, nit: Think these can now all just be put in an anonymous namespace, and moved out of the header. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1567: if (!self) return; nit: Put return on next line.
Thanks for the review. I added some explanations and potential resolution ideas. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:160: HostResolver* CreateNonCachingSystemHostResolver(size_t max_concurrent_resolves, On 2012/01/09 16:05:18, cbentzel wrote: > This should share a helper function with CreateSystemHostResolver - lots of > duplicate logic. Yup, just copied-and-pasted to make ConnectionTester work. Will fix. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:477: DCHECK(origin_loop_->BelongsToCurrentThread()); On 2012/01/09 16:05:18, cbentzel wrote: > This looks incorrect. It will return true even if the resolution completes > successfully and was never explicitly canceled, due to the reset at the end of > OnLookupComplete. This will be bad for statistics gathering on re-issues. True. I added callback reset at the end of OnLookupComplete to avoid re-entrancy issues, but looking at OnProcTaskComplete, I think it will be fine to remove the reset. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1014: PrioritizedDispatcher::Handle& handle() { On 2012/01/09 16:05:18, cbentzel wrote: > non-const ref returns like these are uncommon (and perhaps disallowed). I'd much > rather see a set_handle() Currently HostResolverImpl is the only user of |handle_|. I'm considering moving the logic that uses the handle into the Job. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1049: req->MarkAsCanceled(); On 2012/01/09 16:05:18, cbentzel wrote: > What's the rationale for marking as canceled instead of removing the Request > from the list and deleting it? A good amount of the PriorityTracker logic would > get simpler in that case. A Request could be Canceled during the for loop in CompleteRequests, so removing it from the list here would complicate that loop. This is how HostResolverImpl::Job behaved before the refactoring. I don't think changing this behavior would affect the current PriorityTracker. OTOH if PriorityTracker was replaced with a list scan, then actually removing it would only save one "if (!req->was_cancelled()) continue;". http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1115: base::Bind(&Job::OnProcTaskComplete, base::Unretained(this)), On 2012/01/09 16:05:18, cbentzel wrote: > I wonder if the callback creation and bound method should be on the > HostResolverImpl. It would minimize the Job->Resolver calls, and tend to keep > them Resolver->Job. True, but then the innards of Job (the fact that Job runs a ProcTask and maybe later a DnsTask) are exposed to HostResolverImpl. The current number of Job->Resolver calls is 2, but could be made 1 by moving calling RemoveJob from within OnJobFinished. I'm not counting resolver_->proc_params_. If that should be removed, I'd make it an argument to the constructor. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1131: // new job with the same key in case one of the OnComplete callbacks decides On 2012/01/09 16:05:18, cbentzel wrote: > I wonder if we could do this after iterating through the requests? I'd like to > see fewer paths for Job completion than currently exist, and will try to think > some more about how to make that happen. I'm not sure we can reduce the number of CompleteRequests calls without changing the HostResolver API. We need to call Request::OnComplete when: 1) the Request finishes; 2) the Job is Aborted (due to a network change); 3) the Job is Evicted (due to queue overflow). All of them can be asynchronous and all of them could lead to new Jobs being dispatched, see HostResolverImplTest.StartWithinCallback. We could avoid the complexity in CompleteRequests, if we called MessageLoop::Post(Bind(&Request::OnComplete, Owned(req))). This way all the re-entrancy issues would go away. What would the price of that be? Performance? http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1155: // callback. If it was, we could continue, but we choose to bail. On 2012/01/09 16:57:24, Matt Menke wrote: > Why do we need to allow callbacks to be able to delete the HostResolverImpl, > anyways? Excellent question. See: HostResolverImplTest.DeleteWithinCallback. I haven't yet figured out from just scanning the code if this is really needed. I think I'll just break the behavior and see if this could still be an issue. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1294: if (dispatcher_.num_queued_jobs() > max_queued_jobs_) { On 2012/01/09 16:05:18, cbentzel wrote: > Do you want to check if job->handle().is_null() here? Potentially want to evict > only if we added something to the queue. We could check that but that would be redundant. The queue should never have > max_queued_jobs_, and if it does that means we have added. I'm okay with a DCHECK for sanity. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1295: Job* evicted = static_cast<Job*>(dispatcher_.EvictOldestLowest()); On 2012/01/09 16:05:18, cbentzel wrote: > Why do you need to do the static_cast here? If you kept as base class, would the > evicted == job compare against the base job class? I only need to cast to call evicted->OnEvicted() which is not a part of the PrioritizedDispatcher::Job interface. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1493: NetLog::TYPE_HOST_RESOLVER_IMPL_REQUEST, On 2012/01/09 16:57:24, Matt Menke wrote: > Random comment: Wonder if it would be worth considering not logging the > requests. Suppose we'd lose speculative requests, particularly cache hits if we > did that, so maybe not a great idea. Wouldn't belong in this CL, regardless. > > Suppose could log fake Job entries instead of Request entries instead. Maybe be > better off just keeping it as it is. I was looking into ridding the log from REQUEST entries, but haven't found a good looking solution for ResolveFromCache. More importantly, I noticed that a lot of requests lack a valid net_log source, so I think I'd try to fix that as well.
http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1049: req->MarkAsCanceled(); On 2012/01/09 17:06:59, szym wrote: > On 2012/01/09 16:05:18, cbentzel wrote: > > What's the rationale for marking as canceled instead of removing the Request > > from the list and deleting it? A good amount of the PriorityTracker logic > would > > get simpler in that case. > > A Request could be Canceled during the for loop in CompleteRequests, so removing > it from the list here would complicate that loop. This is how > HostResolverImpl::Job behaved before the refactoring. > > I don't think changing this behavior would affect the current PriorityTracker. > OTOH if PriorityTracker was replaced with a list scan, then actually removing it > would only save one "if (!req->was_cancelled()) continue;". Thanks for the explanation - I agree that it makes the looping operation simpler. http://codereview.chromium.org/9101011/diff/3003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1295: Job* evicted = static_cast<Job*>(dispatcher_.EvictOldestLowest()); On 2012/01/09 17:06:59, szym wrote: > On 2012/01/09 16:05:18, cbentzel wrote: > > Why do you need to do the static_cast here? If you kept as base class, would > the > > evicted == job compare against the base job class? > > I only need to cast to call evicted->OnEvicted() which is not a part of the > PrioritizedDispatcher::Job interface. Yup, I didn't see that. Thanks.
cbentzel, mmenke: Thanks for the review. eroman: PTAL. This CL does not change the external behavior of HostResolverImpl in any way, but it changes the internals significantly. In particular, JobPool has been replaced with net/base/prioritized_dispatcher.h which uses slot reservation to ensure low-priority requests cannot clog the pipe. This will not take effect until we decide to set Limits::reserved_slots.
On 2012/01/09 23:45:18, szym wrote: > cbentzel, mmenke: Thanks for the review. > > eroman: PTAL. This CL does not change the external behavior of HostResolverImpl > in any way, but it changes the internals significantly. In particular, JobPool > has been replaced with net/base/prioritized_dispatcher.h which uses slot > reservation to ensure low-priority requests cannot clog the pipe. This will not > take effect until we decide to set Limits::reserved_slots. eroman: ping
LGTM I did not review the NetLog changes in depth - please have Matt approve those. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl.h File net/base/host_resolver_impl.h (right): http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.h:104: // If |cache| is NULL, then no caching is used. Otherwise we take Probably for another CL: Could change this to use scoped_ptr::Pass http://codereview.chromium.org/9101011/diff/11001/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9101011/diff/11001/net/base/net_log_event_type... net/base/net_log_event_type_list.h:55: // Is AddressList done here for the success END case?
http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:143: os_error); Seems a little weird that SystemHostResolverProc is defined in host_resolver_proc.h, is named SystemHostResolverProc, and is not actually a subclass of HostResolverProc. Might make more sense to put this class in host_resolver.h, though feel free to leave it here if you prefer. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:321: // Returns true if priority changed. Fix comment. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:329: // Returns true if priority changed. Fix comment. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:335: if ((counts_[req_priority] == 0u) && (highest_priority_ == req_priority)) { This initial check doesn't gain us anything, since the loop should handle this case anyways. Could also remove some of the code below and end up with something like: for (size_t i = highest_priority_; i < NUM_PRIORITIES && counts_[i] == 0u; ++i) { highest_priority_ = static_cast<RequestPriority>(i); } We lose out on the DCHECK, but not sure that gets us a whole lot, anyways. Could add a "DCHECK(highest_priority_ < NUM_PRIORITIES - 1 || counts_[NUM_PRIORITIES - 1] == total_count_)", I suppose. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:340: DCHECK_EQ(0u, total_count_); This DCHECK is incorrect, since you don't have a return in the above loop when you find anything. You might want to add a unit_test for this as well - not so much to check the DCHECK, but because it's a case they never currently run into. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:362: // further optimized, but 8 is what FF currently does. Does Firefox retry like we do, on different threads, if the first request is taking too long? Just mention it because it increases the number of simultaneous resolves we have, so could be an issue. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:363: static const size_t kDefaultMaxJobs = 8u; nit: I suggest you put this in an anonymous namespace up top. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:530: job_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK, Elsewhere we generally use begin/end events for this kind of nested event. Think doing so here would make event ordering a little clearer, unless you think we may in some cases race the sync and async resolvers. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:638: void OnCheckForComplete() { nit: While you're here, maybe rename this function? Think something along the lines of "RetryIfNotComplete" or even just "Retry" would be clearer. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:693: // We will record data for the first attempt. Don't contaminate with retry Know this is old code, but I'd suggest a if (!was_completed()) check here. As things are, if a request is canceled and completed, we could record these histograms more than once. Probably a rare case, but it's still a little weird. Feel free to address this in another CL. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1004: net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL); nit: Since you're already logging ERR_ABORTED, not sure there's any reason to also log a cancelled event for the same source. We generally only log one or the other. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:1501: if (job->net_error() == ERR_ABORTED) If we're not writing ERR_ABORTED to the cache, it's a little weird to write ERR_HOST_RESOLVER_QUEUE_TOO_LARGE to it. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... File net/base/host_resolver_impl_unittest.cc (right): http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl_unittest.cc:46: kMaxRetryAttempts); nit: Fix indent. http://codereview.chromium.org/9101011/diff/11001/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9101011/diff/11001/net/base/net_log_event_type... net/base/net_log_event_type_list.h:143: EVENT_TYPE(HOST_RESOLVER_IMPL_JOB_ATTACH) nit: I suggest you use different events for the request and the job. Maybe HOST_RESOLVER_IMPL_REQUEST_ATTACH / HOST_RESOLVER_IMPL_REQUEST_DETACH for the job. Just makes the relationship a little more explicit. http://codereview.chromium.org/9101011/diff/11001/net/base/net_log_event_type... net/base/net_log_event_type_list.h:145: // This is logged for a request when it's canceled and detached from a nit: It's logged for the job when a request is cancelled and detached. http://codereview.chromium.org/9101011/diff/11001/net/base/net_log_event_type... net/base/net_log_event_type_list.h:156: // The creation/completion of a HostResolverImpl::ProcTask to call getaddrinfo. There's also an event of the same name logged for the job when we create the ProcTask, which has yet more parameters not mentioned here. Suggest you rename it (And then document it) to keep the docs here from getting too complicated. http://codereview.chromium.org/9101011/diff/11001/net/base/net_log_source_typ... File net/base/net_log_source_type_list.h (right): http://codereview.chromium.org/9101011/diff/11001/net/base/net_log_source_typ... net/base/net_log_source_type_list.h:28: SOURCE_TYPE(HOST_RESOLVER_IMPL_PROC_TASK, 18) nit: Use the same order here as in chrome/browser/net/passive_log_collector.h. Despite the implication here that the exact mapping matters, it actually shouldn't.
http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:335: if ((counts_[req_priority] == 0u) && (highest_priority_ == req_priority)) { On 2012/01/26 16:54:10, Matt Menke wrote: > This initial check doesn't gain us anything, since the loop should handle this > case anyways. > > Could also remove some of the code below and end up with something like: > > for (size_t i = highest_priority_; i < NUM_PRIORITIES && counts_[i] == 0u; ++i) > { > highest_priority_ = static_cast<RequestPriority>(i); > } > Oops...that code is wrong. Anyways, seems like you shouldn't need quite so much complexity here.
http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:143: os_error); On 2012/01/26 16:54:10, Matt Menke wrote: > Seems a little weird that SystemHostResolverProc is defined in > host_resolver_proc.h, is named SystemHostResolverProc, and is not actually a > subclass of HostResolverProc. > > Might make more sense to put this class in host_resolver.h, though feel free to > leave it here if you prefer. I share your sentiment. I'll asses the necessary changes. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:335: if ((counts_[req_priority] == 0u) && (highest_priority_ == req_priority)) { On 2012/01/26 16:54:10, Matt Menke wrote: > This initial check doesn't gain us anything, since the loop should handle this > case anyways. You are correct. It saves only the assignment to |highest_priority_| in case it doesn't change. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:340: DCHECK_EQ(0u, total_count_); On 2012/01/26 16:54:10, Matt Menke wrote: > This DCHECK is incorrect, since you don't have a return in the above loop when > you find anything. You might want to add a unit_test for this as well - not so > much to check the DCHECK, but because it's a case they never currently run into. Good catch. Need a test for the case the last Request for a running Job is cancelled. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:362: // further optimized, but 8 is what FF currently does. On 2012/01/26 16:54:10, Matt Menke wrote: > Does Firefox retry like we do, on different threads, if the first request is > taking too long? Just mention it because it increases the number of > simultaneous resolves we have, so could be an issue. This comment was moved without change, but you are right that our pre-emptive retries are potentially increasing the number of concurrent resolves. That said, this is probably rather rare. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:530: job_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK, On 2012/01/26 16:54:10, Matt Menke wrote: > Elsewhere we generally use begin/end events for this kind of nested event. > Think doing so here would make event ordering a little clearer, unless you think > we may in some cases race the sync and async resolvers. What concerned me was the possibility of overlap between a DnsTransaction and ProcTask. Hence DnsTransaction has it's own NetLog::Source as well. I realize this increases noise in the net-internals, so I'm fine degrading it to a nested event for now. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:693: // We will record data for the first attempt. Don't contaminate with retry On 2012/01/26 16:54:10, Matt Menke wrote: > Know this is old code, but I'd suggest a if (!was_completed()) check here. As > things are, if a request is canceled and completed, we could record these > histograms more than once. Probably a rare case, but it's still a little weird. > Feel free to address this in another CL. I feel this code should be simplified even further. It is actually not possible for a completed ProcTask to get canceled. (OnProcTaskComplete releases ProcTask.) But it's not apparent from the code, and I got lost in it myself before. http://codereview.chromium.org/9101011/diff/11001/net/base/net_log_event_type... File net/base/net_log_event_type_list.h (right): http://codereview.chromium.org/9101011/diff/11001/net/base/net_log_event_type... net/base/net_log_event_type_list.h:55: // On 2012/01/25 19:05:58, cbentzel wrote: > Is AddressList done here for the success END case? AddressList is listed in HOST_RESOLVER_IMPL_JOB END phase. I don't think it is necessary to copy it to every attached Request.
http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:530: job_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK, On 2012/01/27 04:58:12, szym wrote: > On 2012/01/26 16:54:10, Matt Menke wrote: > > Elsewhere we generally use begin/end events for this kind of nested event. > > Think doing so here would make event ordering a little clearer, unless you > think > > we may in some cases race the sync and async resolvers. > > What concerned me was the possibility of overlap between a DnsTransaction and > ProcTask. Hence DnsTransaction has it's own NetLog::Source as well. I realize > this increases noise in the net-internals, so I'm fine degrading it to a nested > event for now. Even if we do both at once, it may still might actually make sense to use begin events. It'll draw attention to the end event, and as long as we're careful not to log anything after the Job's final end event, we'll be fine, event if the events don't perfectly match. That having been said, I'll defer to you on that when the time comes.
http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:530: job_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_TASK, On 2012/01/27 05:08:44, Matt Menke wrote: > On 2012/01/27 04:58:12, szym wrote: > > On 2012/01/26 16:54:10, Matt Menke wrote: > > > Elsewhere we generally use begin/end events for this kind of nested event. > > > Think doing so here would make event ordering a little clearer, unless you > > think > > > we may in some cases race the sync and async resolvers. > > > > What concerned me was the possibility of overlap between a DnsTransaction and > > ProcTask. Hence DnsTransaction has it's own NetLog::Source as well. I realize > > this increases noise in the net-internals, so I'm fine degrading it to a > nested > > event for now. > > Even if we do both at once, it may still might actually make sense to use begin > events. It'll draw attention to the end event, and as long as we're careful not > to log anything after the Job's final end event, we'll be fine, event if the > events don't perfectly match. > > That having been said, I'll defer to you on that when the time comes. Sorry, that should be it draws attention to the begin events.
Thanks for the review. PTAL at the new logic of ProcTask::OnLookupComplete. http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:143: os_error); On 2012/01/27 04:58:12, szym wrote: > On 2012/01/26 16:54:10, Matt Menke wrote: > > Seems a little weird that SystemHostResolverProc is defined in > > host_resolver_proc.h, is named SystemHostResolverProc, and is not actually a > > subclass of HostResolverProc. > > > > Might make more sense to put this class in host_resolver.h, though feel free > to > > leave it here if you prefer. > > I share your sentiment. I'll asses the necessary changes. There are 6 locations, 3 in tests, 1 in mock. I'm bumping this to another CL. Replacing the function SystemHostResolverProc with a HostResolverProc subclass will add one of: - overhead of HostResolverProc ctor, e.g., SystemHostResolverProc().Resolve(...) - overhead of storing the instance of SystemHostResolverProc, e.g., scoped_refptr<SystemHostResolverProc> - overhead of a vtable call http://codereview.chromium.org/9101011/diff/29001/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (left): http://codereview.chromium.org/9101011/diff/29001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:553: if (!was_cancelled()) { This logic was a bit confusing. I think the new logic preserves the exact behavior with the exception of recording histograms before NetLog.
I broke something and tests are failing. Fixing.
LGTM http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/9101011/diff/11001/net/base/host_resolver_impl... net/base/host_resolver_impl.cc:143: os_error); On 2012/02/01 17:23:51, szym wrote: > On 2012/01/27 04:58:12, szym wrote: > > On 2012/01/26 16:54:10, Matt Menke wrote: > > > Seems a little weird that SystemHostResolverProc is defined in > > > host_resolver_proc.h, is named SystemHostResolverProc, and is not actually a > > > subclass of HostResolverProc. > > > > > > Might make more sense to put this class in host_resolver.h, though feel free > > to > > > leave it here if you prefer. > > > > I share your sentiment. I'll asses the necessary changes. > > There are 6 locations, 3 in tests, 1 in mock. I'm bumping this to another CL. > Replacing the function SystemHostResolverProc with a HostResolverProc subclass > will add one of: > - overhead of HostResolverProc ctor, e.g., SystemHostResolverProc().Resolve(...) > - overhead of storing the instance of SystemHostResolverProc, e.g., > scoped_refptr<SystemHostResolverProc> > - overhead of a vtable call You could still keep the current SystemHostResolverProc function, but rename it, if you wanted.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/9101011/33016
Presubmit check for 9101011-33016 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** License must match: .*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.( \*/)?\n Found a bad license header in these files: chrome/browser/resources/net_internals/events_view.css ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/resources/net_internals/source_entry.js,chrome/browser/net/connection_tester.cc,chrome/browser/net/passive_log_collector.cc,chrome/browser/net/passive_log_collector.h,chrome/browser/resources/net_internals/events_view.css Presubmit checks took 2.1s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
Sorry, LGTMed it from the wrong account.
On 2012/02/01 20:06:42, Matt Menke wrote: > Sorry, LGTMed it from the wrong account. Thanks! Any ideas how to fix the license on events_view.css?
On 2012/02/01 20:07:39, szym wrote: > On 2012/02/01 20:06:42, Matt Menke wrote: > > Sorry, LGTMed it from the wrong account. > > Thanks! Any ideas how to fix the license on events_view.css? I'm not sure. I'd try checking the line ending encoding. You could also try changing the comment style to: /* Copyright (c) 2012 The Chromium Authors. All rights reserved. * ... */
On 2012/02/01 20:11:34, Matt Menke wrote: > On 2012/02/01 20:07:39, szym wrote: > > On 2012/02/01 20:06:42, Matt Menke wrote: > > > Sorry, LGTMed it from the wrong account. > > > > Thanks! Any ideas how to fix the license on events_view.css? > > I'm not sure. I'd try checking the line ending encoding. You could also try > changing the comment style to: > > /* Copyright (c) 2012 The Chromium Authors. All rights reserved. > * ... > */ I see the problem with the regexp. It expects each line to lead with at least one space.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/9101011/33023
Change committed as 120114 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
