[Webkit-unassigned] [Bug 27165] Connections-per-host should be tracked closer to the ResourceHandle level, not the Cache

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27165





--- Comment #9 from Nate Chapin <japhet at chromium.org>  2010-09-20 12:42:46 PST ---
Sorry for not responding sooner, I forgot to cc: myself to thread :(

As posted, this patch is failing fast/loader/early-load-cancel.html because I'm miscounting increments/decrements in some cancelled load cases.  Will fix that in the next version I post.

(In reply to comment #3)
> -        // For named hosts - which are only http(s) hosts - we should always enforce the connection limit.
> -        // For non-named hosts - everything but http(s) - we should only enforce the limit if the document isn't done parsing 
> -        // and we don't know all stylesheets yet.
> 
> This change isn't explained in ChangeLog. Is this really OK to remove? I'm not sure why this is here offhand, but svn history likely has an explanation.

I'm not sure it's safe if the concern is non-web content.  I'll add it back in (at the new location).

Do we want a rule that local urls should never be throttled?

(In reply to comment #4)
> (From update of attachment 67919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67919&action=prettypatch
> 
> This is massively more elegant.  Is there going to be an issue now that more loads will count against the open connection limit?

AFAICT, no.  The only potential issue that I see is that there's no longer any prioritization of un-throttling of requests (previously, when we hit the throttle point, we just stop creating SubresourceLoaders and started again later with the highest priority requests.  Now they're all on timers).

> 
> It seems slightly unfortunate to have the OpenConnectionLimiter be static.  It seems like we might want to hang it as an object off the NetworkContext object, but we can think about that in a future patch.
> 
> > WebCore/loader/ResourceLoader.cpp:132
> > +        m_deferredRequest = clientRequest;
> > +        setDefersLoading(true);
> 
> Should these two lines be one atomic operation?  I'd have to look at how these two pieces of state interact in the rest of this class.

They're non-atomic in other places in ResourceLoader.  I could change that if you think it's important, but setDefersLoading assumes m_deferredRequest is already set.

> 
> > WebCore/loader/OpenConnectionLimiter.cpp:70
> > +    int requestCount = requestsPerHostMap().isEmpty() ? 1 : requestsPerHostMap().get(host) + 1;
> 
> I don't quite understand this line.  Why do we need to branch on requestsPerHostMap().isEmpty() ?  Do you mean to ask if the requestsPerHostMap map has an entry for |host| ?
> 
> > WebCore/loader/OpenConnectionLimiter.cpp:97
> > +    unsigned requestCount = requestsPerHostMap().isEmpty() ? 0 : requestsPerHostMap().get(url.host());
> 
> Same question.

These are leftovers from earlier testing that I forgot to remove.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.