Bug 27165

<rdar://problem/7050112>

Created attachment 67919 [details] Move connections-per-host counting to ResourceLoader I took a stab at this as part of general loader cleanup. Note that I didn't go all the way down to ResourceHandle, so we'd still be missing a few types of loads.

- // 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.

Comment on attachment 67919 [details] Move connections-per-host counting to ResourceLoader 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? 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. > 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.

Here(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. Here's the most recent rationale for that code that I could find: https://bugs.webkit.org/show_bug.cgi?id=27822 There might be more specific information hiding in rdar.

> Is there going to be an issue now that more loads will count against the open connection limit? The number of connections is limited internally by CFNetwork, and likely by other network back-ends, too. Counting them more precisely just lets us do better scheduling in WebCore. Making no more than the number of requests that's supported by the network back-end also makes its job easier, so we don't run into certain bugs.

> There might be more specific information hiding in rdar. This is about compatibility with Adobe installer (specifically, Creative Suite 4). It was quite subtle, and someone will just have to re-test for regression - ideally, before this patch is landed. And then we'd have to test MS Outlook Web Access, because it also needed super subtle tweaks in this code. I don't think I have access to the latter, but I could test the former for you, if needed. We can discuss details on IRC.

Comment on attachment 67919 [details] Move connections-per-host counting to ResourceLoader View in context: https://bugs.webkit.org/attachment.cgi?id=67919&action=review >> 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| ? test

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.

Created attachment 68132 [details] Address comments + don't decrement in releaseResources() if m_request.isNull()

Comment on attachment 68132 [details] Address comments + don't decrement in releaseResources() if m_request.isNull() View in context: https://bugs.webkit.org/attachment.cgi?id=68132&action=review Looks great. Thanks. This is subtle stuff, so it seems likely we've screwed at least one thing up. > WebCore/loader/ResourceLoader.cpp:133 > + // For http(s) hosts, we should always enforce the connection limit. > + // For non-http(s) hosts, we should only enforce the limit if the document isn't done parsing and we don't know all stylesheets yet. > + bool shouldLimitRequests = clientRequest.url().protocolInHTTPFamily() || (m_frame->document() && (m_frame->document()->parsing() || !m_frame->document()->haveStylesheetsLoaded())); m_frame->document() is always non-NULL. Did you verify that this piece of complexity is still needed? > WebCore/loader/OpenConnectionLimiter.h:45 > +}; Missing newline after this line. > WebCore/loader/OpenConnectionLimiter.cpp:70 > + requestsPerHostMap().set(host, requestsPerHostMap().get(host) + 1); There's now operator[] on this object that returns a non-const reference? requestsPerHostMap()[host]++ ?

(In reply to comment #11) > > WebCore/loader/OpenConnectionLimiter.cpp:70 > > + requestsPerHostMap().set(host, requestsPerHostMap().get(host) + 1); > > There's now operator[] on this object that returns a non-const reference? > > requestsPerHostMap()[host]++ ? Are you sure about this? I can't find it anywhere in the HashMap implementation.

(In reply to comment #12) > (In reply to comment #11) > > > WebCore/loader/OpenConnectionLimiter.cpp:70 > > > + requestsPerHostMap().set(host, requestsPerHostMap().get(host) + 1); > > > > There's now operator[] on this object that returns a non-const reference? > > > > requestsPerHostMap()[host]++ ? > > Are you sure about this? I can't find it anywhere in the HashMap implementation. Sorry, that was meant as a question, but I fat-fingered "not" into "now". :)

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > > WebCore/loader/OpenConnectionLimiter.cpp:70 > > > > + requestsPerHostMap().set(host, requestsPerHostMap().get(host) + 1); > > > > > > There's now operator[] on this object that returns a non-const reference? > > > > > > requestsPerHostMap()[host]++ ? > > > > Are you sure about this? I can't find it anywhere in the HashMap implementation. > > Sorry, that was meant as a question, but I fat-fingered "not" into "now". :) I no longer feel like I'm missing something obvious :) HashMap doesn't have any functions that return a MappedType&, it appears to always be a copy. I'll leave it as is unless you object strenuously.

> HashMap doesn't have any functions that return a MappedType&, it appears to always be a copy. I'll leave it as is unless you object strenuously. SGTM

As far as I can see this patch wipes out the request prioritization in the Loader which is the whole purpose of throttling the requests in the first place. This will lead to massive loading performance regression on any platform that doesn't do separate prioritization in the networking layer level. Please explain why this makes sense. I'm r-'n this for now.

> As far as I can see this patch wipes out the request prioritization in the Loader which is the whole purpose of throttling the requests in the first place. Really? I must have misunderstood how the prioritization works. > Please explain why this makes sense. Losing the prioritization makes obviously doesn't make sense, but counting requests at the loader level is pretty non-sensical. Notice that all the folks who are peers with loader have to do a crazy dance to interface with the connection limiter. I suspect the right solution is to either to move the prioritization logic lower in the network stack or to teach the loader how to read back information about the state of the open connection limiter.

(In reply to comment #17) > > As far as I can see this patch wipes out the request prioritization in the Loader which is the whole purpose of throttling the requests in the first place. > > Really? I must have misunderstood how the prioritization works. The idea is that Loader only issues a request to lower levels when there is a free pipe to the host available. New high priority resources can appear at any point of loading as a result of script execution or CSS parsing and we this way we can push these resources to the front of the queue. With this patch ordering is done for resources found in a single runloop cycle only. Any high high priority resource (say script) found afterwards will end up at the end of the queue, loaded after all those 10MB images and delaying the first display for a minute. > Losing the prioritization makes obviously doesn't make sense, but counting requests at the loader level is pretty non-sensical. Notice that all the folks who are peers with loader have to do a crazy dance to interface with the connection limiter. > > I suspect the right solution is to either to move the prioritization logic lower in the network stack or to teach the loader how to read back information about the state of the open connection limiter. If all networking libraries would support per-host prioritization properly then we could just pass the priorities downward and wipe out most of the prioritization related code. That would be the elegant solution. I don't think that is true atm though. What has been discussed, but not implemented yet, is that more loading (XHR, main document,..) should be happening through Loader instead of using ResourceLoaders directly. There would give other benefits too (memory caching, ability to cache parsed forms, validation support...) Moving loading queues as well as the request counting to ResourceLoader level might indeed make sense. Did this patch solve any concrete problems or is it purely about aesthetics?

> Did this patch solve any concrete problems or is it purely about aesthetics? The concrete goal is to make the loader not insane. Here's a diagram of the current state of the loader: https://docs1.google.com/drawings/edit?id=1V3JZltHfNU0HN9bPlLrHOC_yYTriPgv4Dv-LRamymFU&hl=en Here's a sketch of one possible future for the loader: https://docs0.google.com/drawings/edit?id=1ko0LFteYpoXdmfYO1rYme6t-QXLPQdI1Z_ysejpOVYk&hl=en That diagram is missing a lot of details and certainly still up for discussion. Our thought process is to try to improve the loader iteratively. This patch is one step in that direction. Of course, the loader is complicated, so we're try to proceed slowly and carefully. Input from folks such as yourself is much appreciated.

Reading the comments above, it was clearly known that this is patch could change behavior in poorly understood ways. That's not good, especially for refactoring patches. > That diagram is missing a lot of details and certainly still up for discussion. Our thought process is to try to improve the loader iteratively. This patch is one step in that direction. Of course, the loader is complicated, so we're try to proceed slowly and carefully. Input from folks such as yourself is much appreciated. The design looks good as far as pictures of boxes go, though obviously it doesn't really capture the real complexity of the loading subsystem. I think this patch could be fixed by reverting the changes to Loader, expect for those that are directly related to moving counting of connections to the new class. The OpenConnectionLimiter should probably also signal Loader when there are more slots available (this is currently achieved by calling Loader::nonCacheRequestComplete() from random places). Still, I'm not sure that keeping per-host data in two places (Loader::Host map for queues, OpenConnectionLimiter for the count) instead of one is an improvement. Perhaps we should move most of the Loader::Host to the ResourceLoader level instead?

> Reading the comments above, it was clearly known that this is patch could change behavior in poorly understood ways. That's not good, especially for refactoring patches. The fact that no one understands the loader is certainly a problem for the project. Hopefully we'll be able to improve the design to make it more understandable. > The design looks good as far as pictures of boxes go, though obviously it doesn't really capture the real complexity of the loading subsystem. Definitely. Drawing that picture was more of a high-level organizational roadmap. > I think this patch could be fixed by reverting the changes to Loader, expect for those that are directly related to moving counting of connections to the new class. The OpenConnectionLimiter should probably also signal Loader when there are more slots available (this is currently achieved by calling Loader::nonCacheRequestComplete() from random places). That sounds reasonable. > Still, I'm not sure that keeping per-host data in two places (Loader::Host map for queues, OpenConnectionLimiter for the count) instead of one is an improvement. Perhaps we should move most of the Loader::Host to the ResourceLoader level instead? I certainly think it makes sense to decouple prioritization concerns from caching concerns. It's possible we should put these concerns in a layer above ResourceLoader but below the current next layer. In a sense, that would be generalizing Loader to handle more of the loading subsystem, which I think is the direction you mentioned above.

Created attachment 70122 [details] Rewrite - include scheduling (as well as throttling) in OpenConnectionLimiter

Comment on attachment 70122 [details] Rewrite - include scheduling (as well as throttling) in OpenConnectionLimiter View in context: https://bugs.webkit.org/attachment.cgi?id=70122&action=review This direction is great. Mostly minor aesthetic comments and a question. > WebCore/loader/MainResourceLoader.cpp:553 > + openConnectionLimiter()->addRequestWithoutSchedule(this); openConnectionLimiter()->addRequest(DoNotSchedule) ? > WebCore/loader/OpenConnectionLimiter.h:2 > + * Copyright (C) 2010 Google Inc. All rights reserved. You probably need the copyright lines from Loader.h/cpp in these files. > WebCore/loader/OpenConnectionLimiter.h:47 > +class OpenConnectionLimiter : public Noncopyable { Maybe OpenConnectionLimiter -> ResourceLoadScheduler ? > WebCore/loader/OpenConnectionLimiter.h:49 > + friend OpenConnectionLimiter* openConnectionLimiter(); Interesting. Yeah, I think that's the pattern we use here. Other code bases would make this a static function, but this seems fine. > WebCore/loader/OpenConnectionLimiter.h:64 > + ~OpenConnectionLimiter(); Add a blank line below here. > WebCore/loader/OpenConnectionLimiter.h:70 > + class Host : public RefCounted<Host> { Host is a confusing name. Maybe HostInformation? This class is just data-only, right? > WebCore/loader/OpenConnectionLimiter.h:81 > + void servePendingRequests(Priority minimumPriority = VeryLow); I see, this isn't model-like. Can we move this into the parent class? That way the parent class is the controller and this class is the model. Also, is there a big benefit to having this be an inner class? It seems like it would be easier if it had it's own header and file. > WebCore/loader/OpenConnectionLimiter.h:82 > + void end(ResourceLoader*, OpenConnectionLimiter::Priority); OpenConnectionLimiter:: isn't needed here. > WebCore/loader/ResourceLoader.h:133 > + friend class OpenConnectionLimiter; sadface > WebCore/loader/OpenConnectionLimiter.cpp:75 > + static OpenConnectionLimiter* openConnectionLimiter = new OpenConnectionLimiter(); DECLARE_STATIC_LOCAL > WebCore/loader/OpenConnectionLimiter.cpp:100 > + m_hosts.checkConsistency(); > + String hostName = url.host(); > + host = m_hosts.get(hostName); > + if (!host) { > + host = Host::create(hostName, maxRequestsInFlightPerHost); > + m_hosts.add(hostName, host); > + } This seems like it should be in a helper function. > WebCore/loader/OpenConnectionLimiter.cpp:128 > + if (url.protocolInHTTPFamily()) { > + m_hosts.checkConsistency(); > + host = m_hosts.get(url.host()); > + } else > + host = m_nonHTTPProtocolHost; Another instance of this url => Host lookup. > WebCore/loader/OpenConnectionLimiter.cpp:145 > + RefPtr<Host> oldHost; > + if (oldURL.protocolInHTTPFamily()) { > + m_hosts.checkConsistency(); > + oldHost = m_hosts.get(oldURL.host()); > + } else > + oldHost = m_nonHTTPProtocolHost; :) > WebCore/loader/OpenConnectionLimiter.cpp:156 > + m_hosts.checkConsistency(); > + String hostName = newURL.host(); > + newHost = m_hosts.get(hostName); > + if (!newHost) { > + newHost = Host::create(hostName, maxRequestsInFlightPerHost); > + m_hosts.add(hostName, newHost); > + } Here's the same thing but with the "create" bit set. > WebCore/loader/OpenConnectionLimiter.cpp:182 > + RefPtr<Host> host; > + KURL url(ParsedURLString, resourceLoader->request().url()); > + if (url.protocolInHTTPFamily()) { > + m_hosts.checkConsistency(); > + String hostName = url.host(); > + host = m_hosts.get(hostName); > + if (!host) { > + host = Host::create(hostName, maxRequestsInFlightPerHost); > + m_hosts.add(hostName, host); > + } > + } else > + host = m_nonHTTPProtocolHost; :) > WebCore/loader/OpenConnectionLimiter.cpp:207 > + host->servePendingRequests(minimumPriority); Yeah, we can probably move this controller logic from Host to here. > WebCore/loader/OpenConnectionLimiter.cpp:299 > +void OpenConnectionLimiter::Host::end(ResourceLoader* resourceLoader, OpenConnectionLimiter::Priority priority) end -> remove ? > LayoutTests/http/tests/xmlhttprequest/logout.html:32 > - xhr.abort(); > + xhr.onload = function() { > + document.getElementById("logout").innerHTML = isAuthenticated() ? "FAIL" : "PASS"; > + > + if (window.layoutTestController) > + layoutTestController.notifyDone(); > + } Is our new behavior more consistent or less consistent with other browsers?

> > LayoutTests/http/tests/xmlhttprequest/logout.html:32 > > - xhr.abort(); > > + xhr.onload = function() { > > + document.getElementById("logout").innerHTML = isAuthenticated() ? "FAIL" : "PASS"; > > + > > + if (window.layoutTestController) > > + layoutTestController.notifyDone(); > > + } > > Is our new behavior more consistent or less consistent with other browsers? Assuming the test isn't rewritten, the output for the last PASS/FAIL: FF3/4 passes IE8 passes (but fails a different part) IE9 fails Chrome fails already due to another bug Safari passes, but will fail with this change

Created attachment 70314 [details] Rename to ResourceLoadScheduler, etc.

Comment on attachment 70314 [details] Rename to ResourceLoadScheduler, etc. View in context: https://bugs.webkit.org/attachment.cgi?id=70314&action=review > WebKit/mac/WebView/WebView.mm:624 > - (void)_dispatchPendingLoadRequests > { > - cache()->loader()->servePendingRequests(); > + resourceLoadScheduler()->servePendingRequests(); > } I wonder why only Mac does this. > WebCore/loader/ResourceLoadScheduler.cpp:66 > +ResourceLoadScheduler::HostInformation* ResourceLoadScheduler::hostForURL(const KURL& url, bool createIfNotFound) using an enum here is probably slightly more readable at the callsites. > WebCore/loader/ResourceLoadScheduler.cpp:71 > + RefPtr<HostInformation> host; Is there any reason to declare this up here instead of on line 74 where it's initialized? > WebCore/loader/ResourceLoadScheduler.cpp:153 > + HostMap::iterator i = m_hosts.begin(); We'd usually call this variable iter to remind ourselves that it's not an index. > WebCore/loader/ResourceLoadScheduler.cpp:158 > + for (unsigned n = 0; n < hostsToServe.size(); ++n) { We'd usually call this one "i" because it is an index. "n" is usually a constant and the total number of things. > WebCore/loader/ResourceLoadScheduler.h:34 > +namespace WebCore { Missing a blank line after this line. > WebCore/loader/loader.cpp:-133 > - AtomicString hostName = url.host(); Does it matter that this used to be an AtomicString? > WebCore/loader/loader.cpp:225 > - m_requestsLoading.remove(loader); > loader->clearClient(); > + m_requestsLoading.remove(loader); Why this re-ordering?

Comment on attachment 70314 [details] Rename to ResourceLoadScheduler, etc. I think this is great. My only concern is the issue mentioned in Comment #24. I think we should land this (maybe with the nits above addressed). If the comment #24 issue turns out to be a problem, we'll figure something out.

Antti, would you like to look over this again before landing?

Oh, and I just noticed that this fails a test I've written, so I'd like to take a closer look, too. Please don't land this yet.

(In reply to comment #29) > Oh, and I just noticed that this fails a test I've written, so I'd like to take a closer look, too. Please don't land this yet. No problem, I was planning on waiting until Monday morning anyway.

Comment on attachment 70314 [details] Rename to ResourceLoadScheduler, etc. +// Having a limit might still help getting more important resources first This is just copy/pasted, but this comment is nearly incomprehensible. Might? Still? When can we remove the limit? +static const unsigned maxRequestsInFlightForNonHTTPProtocols = 20; I found REQUEST_MANAGEMENT_ENABLED useful before. +static unsigned maxRequestsInFlightPerHost; This has lost an important comment that used to be present in original code (match the parallel connection count used by the networking layer). Ideally, we should be clearer about whether this is about parallel connections or about inflight requests (this is not the same if request pipelining is enabled). +ResourceLoadScheduler::Priority ResourceLoadScheduler::determinePriority(const ResourceRequest& request) const +{ + switch (request.targetType()) { + case ResourceRequest::TargetIsMainFrame: Ugh, this is horrible. ResourceRequest::m_targetType is misguided code - this member variable isn't guaranteed to be accurate. The reason is that Mac WebKit API sends every request to embedder as an NSURLRequest, and the embedder is free to do anything with it. After the call returns, ResourceRequest is re-created from NSURLRequest, so anything ResourceRequest::doUpdateResourceRequest() doesn't know about is lost. It's not good design to have cross-platform code with such strong Mac ties, but that's what we have now (plus misguided code from developers who chose to ignore this limitation after having been told about it). We should at least have comments in ResourceRequestBase.h - or better, come up with a strategy for dealing with this. See also: bug 44722 comment 49. In any case, request.targetType() shouldn't be used in ResourceLoadScheduler now. +ResourceLoadScheduler* resourceLoadScheduler() +{ + DEFINE_STATIC_LOCAL(ResourceLoadScheduler, resourceLoadScheduler, ()); + return &resourceLoadScheduler; +} I'd add ASSERT(isMainThread()) here - we need a lot more of those asserts with additional functionality being available in workers. +ResourceLoadScheduler::ResourceLoadScheduler() + : m_requestTimer(this, &ResourceLoadScheduler::requestTimerFired) + , m_isSuspendingPendingRequests(false) +{ + m_nonHTTPProtocolHost = HostInformation::create(String(), maxRequestsInFlightForNonHTTPProtocols); Why does m_nonHTTPProtocolHost need to be assigned, and not initialized together with m_requestTimer and m_isSuspendingPendingRequests? + HostInformation* oldHost = hostForURL(resourceLoader->request().url()); + ASSERT(oldHost); + HostInformation* newHost = hostForURL(request.url(), true); + + if (oldHost == newHost) + return; Comparing by pointer is slightly surprising - besides AtomicString, this never works. Perhaps a function like urlsAreOnSameHost() would be easier to the eyes? + class HostInformation : public RefCounted<HostInformation> { Why is HostInformation reference counted? It seems to have a clear ownership. + // Try to request important resources immediately Please add periods at the end of sentences, even in moved code. + HostMap::iterator i = m_hosts.begin(); + HostMap::iterator end = m_hosts.end(); + for (;i != end; ++i) There is no reason to define i outside for. Also, I don't see why end is precomputed, but hostsToServe.size() just below the quoted code isn't. - m_handle = ResourceHandle::create(m_frame->loader()->networkingContext(), clientRequest, this, m_defersLoading, m_shouldContentSniff); + resourceLoadScheduler()->add(this); So, ResourceLoader has a circular relationship with ResourceLoadScheduler - you create a ResourceLoader, it registers with ResourceLoadScheduler, and then the scheduler tells the loader to start. And other times, you just start a load and register it with the scheduler after the fact . I didn't expect ResourceLoader to know about scheduling, but perhaps I didn't think about it long enough. Can this be streamlined? + if (!redirectResponse.isNull()) + resourceLoadScheduler()->changeHost(this, request); Since changeHost is only called as a response to a redirect, it should probably have "redirect" in its name somehow. I've been puzzled about what this function did until I saw where it was being called. I'm wondering how changing a host affects scheduling. Clearly, we don't get a chance to re-prioritize the request in WebCore, networking library does everything internally. It may be worth making an experiment - create multiple long-standing connections to host 1, then make a request to host 2 that gets redirected to host 1. Will it load immediately in Safari, Chrome or other browsers, even though the concurrent connection limit is already met? This is probably not going to affect this patch, but is something that's nice to check while we're at it. + enum SchedulePolicy { + Schedule, + AlreadyLoading + }; SchedulePolicy::Schedule looks a little mysterious, maybe ScheduleLoad? I know it's hard to come up with good names for these enums. +ResourceLoadScheduler::HostInformation* ResourceLoadScheduler::hostForURL(const KURL& url, bool createIfNotFound) +{ + if (!url.protocolInHTTPFamily()) + return m_nonHTTPProtocolHost.get(); Should these all be the same (ftp: and data: have little in common)? Is that how existing code works? + const ResourceRequest& request() const { return m_request; } It's a little dangerous to have request() as a public method of ResourceLoader, it's easy to misuse. There are initial request, request as changed by embedder delegate call, and redirected request. There's also m_deferredRequest, which I have no idea about. The fewer "request()" functions a developer has to randomly choose from at any given call site, the better. And if this one has to be made public, it needs a comment explaining which request this is. + * http/tests/xmlhttprequest/logout.html: It is no longer safe + to assume that an async xhr will have any effects synchronously. Did this test pass in Firefox and IE in its original form? If it did, we should probably find a way to keep it working. This patch needs more work (possibly several more iterations), but I really like the goal and the scope of changes that you chose to make.

Looks pretty good. I think the job of determining the resource priority (determinePriority() function) should not be part of the scheduler itself. Instead the priority should be determined in higher level and passed to the scheduler, perhaps as a field in ResourceRequest.

Sorry for the delay....a few comments/questions on the less-trivial requests (In reply to comment #31) > (From update of attachment 70314 [details]) > +ResourceLoadScheduler::Priority ResourceLoadScheduler::determinePriority(const ResourceRequest& request) const > +{ > + switch (request.targetType()) { > + case ResourceRequest::TargetIsMainFrame: > > Ugh, this is horrible. ResourceRequest::m_targetType is misguided code - this member variable isn't guaranteed to be accurate. The reason is that Mac WebKit API sends every request to embedder as an NSURLRequest, and the embedder is free to do anything with it. After the call returns, ResourceRequest is re-created from NSURLRequest, so anything ResourceRequest::doUpdateResourceRequest() doesn't know about is lost. > > It's not good design to have cross-platform code with such strong Mac ties, but that's what we have now (plus misguided code from developers who chose to ignore this limitation after having been told about it). We should at least have comments in ResourceRequestBase.h - or better, come up with a strategy for dealing with this. See also: bug 44722 comment 49. > > In any case, request.targetType() shouldn't be used in ResourceLoadScheduler now. > > > - m_handle = ResourceHandle::create(m_frame->loader()->networkingContext(), clientRequest, this, m_defersLoading, m_shouldContentSniff); > > + resourceLoadScheduler()->add(this); > > So, ResourceLoader has a circular relationship with ResourceLoadScheduler - you create a ResourceLoader, it registers with ResourceLoadScheduler, and then the scheduler tells the loader to start. And other times, you just start a load and register it with the scheduler after the fact . I didn't expect ResourceLoader to know about scheduling, but perhaps I didn't think about it long enough. Can this be streamlined? Removing knowledge of the initial scheduling from ResourceLoader would simplify things and make it easier to keep ResourceLoadScheduler from having to know about how priority is calculated, so I think that part makes sense. However, I don't see a good way to keep ResourceLoader from knowing about the end-of-life accounting (unless we come up with a better time to call ResourceLoadScheduler->remove() than the ResourceLoader destructor). Does it seem reasonable to allow ResourceLoader to know about the cross-origin redirect and remove cases, but not know about how the initial scheduling occurs? > > + if (!redirectResponse.isNull()) > + resourceLoadScheduler()->changeHost(this, request); > > Since changeHost is only called as a response to a redirect, it should probably have "redirect" in its name somehow. I've been puzzled about what this function did until I saw where it was being called. > > I'm wondering how changing a host affects scheduling. Clearly, we don't get a chance to re-prioritize the request in WebCore, networking library does everything internally. > > It may be worth making an experiment - create multiple long-standing connections to host 1, then make a request to host 2 that gets redirected to host 1. Will it load immediately in Safari, Chrome or other browsers, even though the concurrent connection limit is already met? This is probably not going to affect this patch, but is something that's nice to check while we're at it. > Agreed on renaming changeHost(). Currently, I don't think we're handling this case correctly (namely, if we have a cross-origin redirect, we continue to account for it under the original host, not the one that it ought to be under for the redirect). This patch, on the other hand, needs to move the request between hosts because we no longer have a host subclass directly receiving callbacks. I'll see if I can get the experiment for you. > > +ResourceLoadScheduler::HostInformation* ResourceLoadScheduler::hostForURL(const KURL& url, bool createIfNotFound) > +{ > + if (!url.protocolInHTTPFamily()) > + return m_nonHTTPProtocolHost.get(); > > Should these all be the same (ftp: and data: have little in common)? Is that how existing code works? It's how the existing code works. I have no strong opinion on how it should work. > > + * http/tests/xmlhttprequest/logout.html: It is no longer safe > + to assume that an async xhr will have any effects synchronously. > > Did this test pass in Firefox and IE in its original form? If it did, we should probably find a way to keep it working. The data I provided before was for the original form. I can get it for the new form of the test if you like, but I was mostly pointing out that behavior on this test isn't standard as is.

Created attachment 72883 [details] patch #5 (I think) I believe this address all the comments on the previous versions. Note that this version includes a small yak shave that may belong in a separate patch (removing some duplicate code in creation of NetscapePluginStreamLoader and making it behave more like its sibling, SubresourceLoader). Let me know if you'd prefer I break it out.

Looks really good to me. I'll try it out and let others comment too.

Comment on attachment 72883 [details] patch #5 (I think) View in context: https://bugs.webkit.org/attachment.cgi?id=72883&action=review >> Did this test pass in Firefox and IE in its original form? If it did, we should probably find a way to keep it working. > The data I provided before was for the original form. I can get it for the new form of the test if you like, but I was mostly pointing out that behavior on this test isn't standard as is. So, we used to be compatible with Firefox and IE8, and no longer are. This seems quite unfortunate. Perhaps credentials should be stored immediately, not waiting for a request to actually start? Or maybe requests with explicit credentials should always get highest priority, to make sure that subsequent ones are properly authenticated (or logged out)? I don't think this difference necessarily warrants r-, but logging out is such a sensitive area that one gets paranoid. > WebCore/ChangeLog:5 > + Need a short description and bug URL (OOPS!) As it says :) > WebCore/loader/loader.cpp:185 > - LOG(ResourceLoading, "Host '%s' received %s. Current count %d\n", m_name.string().latin1().data(), resource->url().latin1().data(), m_requestsLoading.size()); > + LOG(ResourceLoading, "Received %s.", resource->url().latin1().data()); %s should probably be in single quotes, too (I think I omitted those by mistake when recently adding the logging). > WebCore/loader/loader.cpp:224 > + LOG(ResourceLoading, "Failed to load %s (cancelled=%d).\n", resource->url().latin1().data(), cancelled); Ditto.

(In reply to comment #36) > (From update of attachment 72883 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72883&action=review > > >> Did this test pass in Firefox and IE in its original form? If it did, we should probably find a way to keep it working. > > The data I provided before was for the original form. I can get it for the new form of the test if you like, but I was mostly pointing out that behavior on this test isn't standard as is. > > So, we used to be compatible with Firefox and IE8, and no longer are. This seems quite unfortunate. Perhaps credentials should be stored immediately, not waiting for a request to actually start? Or maybe requests with explicit credentials should always get highest priority, to make sure that subsequent ones are properly authenticated (or logged out)? > > I don't think this difference necessarily warrants r-, but logging out is such a sensitive area that one gets paranoid. I just experimented with the suggestion to give High priority to requests with explicit credentials. It appears that this breaks the test, but in a different way. The test currently sends an async request, then logs out, and expects the async request to return 200. With higher priority for explicit credentials, it looks like we end up sending the logout request before the async request. I'm not sure whether we're worried about that case. I'd be happy to also try your suggestion of storing credentials immediately, but I'm having trouble finding the code where we store the credentials. Is that somewhere in WebCore?

> I'm having trouble finding the code where we store the credentials. Is that somewhere in WebCore? Yes, but it's in platform specific code. For Mac, it's in ResourceHandle::createNSURLConnection(): // If there is already a protection space known for the URL, update stored credentials before sending a request. // This makes it possible to implement logout by sending an XMLHttpRequest with known incorrect credentials, and aborting it immediately // (so that an authentication dialog doesn't pop up). CredentialStorage::set(Credential(d->m_user, d->m_pass, CredentialPersistenceNone), firstRequest().url()); This approach will have the same problem that you described though.

Comment on attachment 72883 [details] patch #5 (I think) I feel like the schedulePluginStreamLoad abstraction part of this could be landed simply w/o any controversy. Maybe that should be done first if review seems blocked here otherwise.

It would be good to get the whole patch landed as it rots fast (it doesn't apply anymore). I can't evaluate how serious this credentials issue is though. Could it be handled afterwards?

Comment on attachment 72883 [details] patch #5 (I think) I think that this patch is flawed in that it changes load scheduling instead of just the promised refactoring. It's the fact that XMLHttpRequest loads do not start right away - even when all loader slots are empty - that causes the test regression. That said, this patch still seems like an improvement all things considered. Given Nate's good track record of thoroughness in particular, I think this is OK to land it and investigate XMLHttpRequest scheduling changes in a follow-up bug.

(In reply to comment #41) > (From update of attachment 72883 [details]) > I think that this patch is flawed in that it changes load scheduling instead of just the promised refactoring. It's the fact that XMLHttpRequest loads do not start right away - even when all loader slots are empty - that causes the test regression. > > That said, this patch still seems like an improvement all things considered. Given Nate's good track record of thoroughness in particular, I think this is OK to land it and investigate XMLHttpRequest scheduling changes in a follow-up bug. I just experimented with bumping XHRs from Low priority to Medium (which guarantees we'll try to start them immediately), and that causes this test to pass. If it's ok with you, I think I'll add changing the ResourceLoaderScheduler::Low to Medium in DocumentThreadableLoader.cpp to the list of changes before submitting. Sorry for not catching this sooner, I really thought I had tried fixing the problem by just increasing the priority of XHRs. Note, however, that this test could theoretically become flaky if we do hit the number of requests where we start throttling.

Sounds good to me.

http://trac.webkit.org/changeset/71562 might have broken Qt Linux Release The following tests are not passing: editing/selection/move-by-line-003.html fast/dom/Document/CaretRangeFromPoint/replace-element.html

Landed: http://trac.webkit.org/changeset/71562 Thanks!

I think this broke windows? Compiling... WebPage.cpp ..\WebProcess\WebPage\WebPage.cpp(121) : error C2259: 'WebKit::WebChromeClient' : cannot instantiate abstract class due to following members: 'void WebCore::ChromeClient::focusedFrameChanged(WebCore::Frame *)' : is abstract C:\cygwin\mnt\svn\webkit-win-ews\WebKitBuild\Include\WebCore/ChromeClient.h(97) : see declaration of 'WebCore::ChromeClient::focusedFrameChanged' PluginView.cpp ..\WebProcess\Plugins\PluginView.cpp(132) : error C2660: 'WebCore::NetscapePlugInStreamLoader::create' : function does not take 2 arguments Generating Code... Project : warning PRJ0018 : The following environment variables were not found: $(PRODUCTION) </pre></table><table width=100% bgcolor=#DFDFE5><tr><td><font face=arial size=+2> Results </font></table><table width=* cellspacing=0 cellpadding=0><tr><td width=0 bgcolor=#EDEDF5>&nbsp;</td><td width=0 bgcolor=#FFFFFF>&nbsp;</td><td width=*><pre>Build log was saved at "file://C:\cygwin\mnt\svn\webkit-win-ews\WebKitBuild\obj\WebKit\Debug\BuildLog.htm" WebKit - 2 error(s), 1 warning(s)

That log is from the win-ews: http://queues.webkit.org/results/5541055. The other win-ews instance seems to be functioning fine, so it's possible the error is just temporary.

(In reply to comment #47) > That log is from the win-ews: http://queues.webkit.org/results/5541055. The other win-ews instance seems to be functioning fine, so it's possible the error is just temporary. The error was fixed in r71548.

(In reply to comment #7) > > There might be more specific information hiding in rdar. > > This is about compatibility with Adobe installer (specifically, Creative Suite 4). It was quite subtle, and someone will just have to re-test for regression - ideally, before this patch is landed. > > And then we'd have to test MS Outlook Web Access, because it also needed super subtle tweaks in this code. I don't think I have access to the latter, but I could test the former for you, if needed. We can discuss details on IRC. This did indeed break the Adobe CS 4 installer :(

(In reply to comment #49) > This did indeed break the Adobe CS 4 installer :( Filed https://bugs.webkit.org/show_bug.cgi?id=52264 to track this