Have NetworkProcess manage resource load scheduling. Now that the LoaderStrategy can provide a custom ResourceLoadScheduler, we can use that to mask IPC to the network process which can manage all of the requests coming from any number of WebProcesses, and dispatch them back to their WebProcesses of origin when it's time. For now the actual loading will still take place in the WebProcesses.
Created attachment 170852 [details] WIP patch Theres some testing left to do on this patch, but it's close to what I will eventually submit Submitting it now to get the EWS ball rolling.
Attachment 170852 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.exp.in', u'Source/W..." exit_code: 1 Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:74: The parameter name "messageID" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:74: The parameter name "decoder" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:77: The parameter name "resourceLoadIdentifier" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:78: The parameter name "resourceLoadIdentifier" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:79: The parameter name "resourceLoadIdentifier" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:80: The parameter name "resourceLoadIdentifier" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:43: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:65: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:168: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:38: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:39: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:68: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:95: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:132: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:196: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:60: The parameter name "resourceLoadIdentifier" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h:55: The parameter name "resourceLoadIdentifier" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.h:31: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/loader/ResourceLoader.h:156: The parameter name "identifier" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 19 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 170852 [details] WIP patch Attachment 170852 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14552958
Created attachment 170952 [details] WIP #2 - Check style and mac build while I start in on changelogs
Attachment 170952 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.exp.in', u'Source/W..." exit_code: 1 Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:77: The parameter name "resourceLoadIdentifier" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:93: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:94: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/loader/ResourceLoader.h:156: The parameter name "identifier" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 170963 [details] Patch v1 Since it's so huge, formally asking for review on this version of the patch while I continue testing.
Attachment 170963 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/loader/ResourceLoader.h:156: The parameter name "identifier" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 170963 [details] Patch v1 Attachment 170963 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14606185 New failing tests: animations/suspend-resume-animation-events.html
(In reply to comment #8) > (From update of attachment 170963 [details]) > Attachment 170963 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/14606185 > > New failing tests: > animations/suspend-resume-animation-events.html This seems dubious as this code doesn't affect chrome or animations.
> This seems dubious as this code doesn't affect chrome or animations. Sorry, that test might be flaky. It looks like another run went cleanly and your patch has a green bubble.
Comment on attachment 170963 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=170963&action=review Looks very reasonable to me. > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:170 > + // 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. > + > + // FIXME: How can the network process know if the document is parsing or has its stylesheets loaded? > + // We'll need to teach it. > + // For now we'll just always limit requests for named hosts, but we need to support the following from ResourceLoadScheduler: I think that some of these requirements are only there for binary compatibility reasons. We might be able to have a simpler behavior with WebKit2. > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:91 > + // We want the network process involved in scheduling data URL loads but it doesn't need to know the full (often long) URL. Why does the network process always need to be involved? > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:114 > + if (!WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::AddLoadInProgress(resourceLoader->url()), Messages::NetworkConnectionToWebProcess::AddLoadInProgress::Reply(identifier), 0)) { Ugh, sync? > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:115 > + // FIXME: What should we do if this fails? Perhaps FIXMEs that need to all be addressed need a special tag, like FIXME (NetworkProcess), to make it easy to search for them and see how far we are from being done. > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.h:56 > + virtual void suspendPendingRequests(); > + virtual void resumePendingRequests(); No OVERRIDE anywhere?
Comment on attachment 170963 [details] Patch v1 These comments are easy to address (mostly with FIXMEs), r=me.
(In reply to comment #10) > > This seems dubious as this code doesn't affect chrome or animations. > > Sorry, that test might be flaky. It looks like another run went cleanly and your patch has a green bubble. Awesome. I didn't know EWS retroactively made red bubbles green :)
(In reply to comment #11) > (From update of attachment 170963 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170963&action=review > > Looks very reasonable to me. > > > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:170 > > + // 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. > > + > > + // FIXME: How can the network process know if the document is parsing or has its stylesheets loaded? > > + // We'll need to teach it. > > + // For now we'll just always limit requests for named hosts, but we need to support the following from ResourceLoadScheduler: > > I think that some of these requirements are only there for binary compatibility reasons. We might be able to have a simpler behavior with WebKit2. That's great news. I've updated the comment to reflect this. > > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:91 > > + // We want the network process involved in scheduling data URL loads but it doesn't need to know the full (often long) URL. > > Why does the network process always need to be involved? Alexey and I discussed in person, but for posterity: Currently, a single WebProcess limits the total number of non-http(s) loads at a time to 20. It does so with the ResourceLoadScheduler. So with one WebProcess we might have up to 20 of these loads. If we leave this scheduling to each WebProcess, with 20 WebProcesses we might have up to *400* of these loads. That's a lot of potentially large data: loads and a lot of potentially competitive disk i/o. We might be able to relax this going forward but - for now - using the shared scheduler maintains the status quo. > > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:114 > > + if (!WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::AddLoadInProgress(resourceLoader->url()), Messages::NetworkConnectionToWebProcess::AddLoadInProgress::Reply(identifier), 0)) { > > Ugh, sync? For now. It's both a conservative decision and due to the nature of needing the identifiers themselves synchronously. I'm adding a FIXME as discussed to track making the identifier requirement mean we can be async. > > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:115 > > + // FIXME: What should we do if this fails? > > Perhaps FIXMEs that need to all be addressed need a special tag, like FIXME (NetworkProcess), to make it easy to search for them and see how far we are from being done. Done. > > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.h:56 > > + virtual void suspendPendingRequests(); > > + virtual void resumePendingRequests(); > > No OVERRIDE anywhere? DOH! Done.
http://trac.webkit.org/changeset/132721
Looks like this caused all the perf tests to fail: http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Perf%29/builds/6544/steps/perf-test/logs/stdio
(In reply to comment #16) > Looks like this caused all the perf tests to fail: > http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Perf%29/builds/6544/steps/perf-test/logs/stdio I'm not familiar with the perf tests. Can someone fill me in on what the expectations are? Without additional exploration, it's surprising to me this should've been a change in behavior in any of our tests as none of the tests should have the network process enabled.
You can run them with ./Tools/Scripts/run-perf-tests. It looks they started crashing.
I think that bug 100602 took care of those crashes.
(In reply to comment #18) > You can run them with ./Tools/Scripts/run-perf-tests. It looks they started crashing. I wonder if it was https://bugs.webkit.org/show_bug.cgi?id=100602, then. They seem to have gone green since http://trac.webkit.org/changeset/132744
(In reply to comment #19) > I think that bug 100602 took care of those crashes. Yah, that. hehe
Comment on attachment 170963 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=170963&action=review > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:182 > +void WebResourceLoadScheduler::suspendPendingRequests() > +{ > + WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::SuspendPendingRequests(), Messages::NetworkConnectionToWebProcess::SuspendPendingRequests::Reply(), 0); > + > + ++m_suspendPendingRequestsCount; > +} > + > +void WebResourceLoadScheduler::resumePendingRequests() > +{ > + WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::ResumePendingRequests(), Messages::NetworkConnectionToWebProcess::ResumePendingRequests::Reply(), 0); > + > + ASSERT(m_suspendPendingRequestsCount); > + --m_suspendPendingRequestsCount; > +} Synchronous IPC here is likely to do bad things to performance. What is the plan for getting rid of it?
(In reply to comment #22) > (From update of attachment 170963 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170963&action=review > > > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:182 > > +void WebResourceLoadScheduler::suspendPendingRequests() > > +{ > > + WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::SuspendPendingRequests(), Messages::NetworkConnectionToWebProcess::SuspendPendingRequests::Reply(), 0); > > + > > + ++m_suspendPendingRequestsCount; > > +} > > + > > +void WebResourceLoadScheduler::resumePendingRequests() > > +{ > > + WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::ResumePendingRequests(), Messages::NetworkConnectionToWebProcess::ResumePendingRequests::Reply(), 0); > > + > > + ASSERT(m_suspendPendingRequestsCount); > > + --m_suspendPendingRequestsCount; > > +} > > Synchronous IPC here is likely to do bad things to performance. What is the plan for getting rid of it? Indeed, I have already seen spin dumps with this in trace.
(In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 170963 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=170963&action=review > > > > > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:182 > > > +void WebResourceLoadScheduler::suspendPendingRequests() > > > +{ > > > + WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::SuspendPendingRequests(), Messages::NetworkConnectionToWebProcess::SuspendPendingRequests::Reply(), 0); > > > + > > > + ++m_suspendPendingRequestsCount; > > > +} > > > + > > > +void WebResourceLoadScheduler::resumePendingRequests() > > > +{ > > > + WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::ResumePendingRequests(), Messages::NetworkConnectionToWebProcess::ResumePendingRequests::Reply(), 0); > > > + > > > + ASSERT(m_suspendPendingRequestsCount); > > > + --m_suspendPendingRequestsCount; > > > +} > > > > Synchronous IPC here is likely to do bad things to performance. What is the plan for getting rid of it? > > Indeed, I have already seen spin dumps with this in trace. Making these synchronous was a cautious step to maintain previous behavior, as no one could really give a definitive answer about just how important these are or what the ramifications would be for them getting "wrong". The plan is either determine that they can be asynchronous with no ill effects, determine that they can be per-WebProcess, or if we have to pretend they are synchronous to come up with a shared memory solution.
I think the suppression mechanism here exists to prevent resource loads from completing synchronously in the middle of style recalc and resulting in re-entry bugs. All loads through network process should be asynchronous so the problems shouldn't exist at all. I think these can be safely removed.
(In reply to comment #25) > I think the suppression mechanism here exists to prevent resource loads from completing synchronously in the middle of style recalc and resulting in re-entry bugs. All loads through network process should be asynchronous so the problems shouldn't exist at all. I think these can be safely removed. To be more specific, I think you mean that in the calls in WebCore should remain but in the NetworkProcess scheduler they should just be no-ops. Correct?
(In reply to comment #26) > To be more specific, I think you mean that in the calls in WebCore should remain but in the NetworkProcess scheduler they should just be no-ops. > > Correct? Are there any mechanism left where a WebKit initiated network load can synchronously go to the client and allow client to synchronously call back to WebKit? If not (and I hope so considering we have an opportunity to fix our APIs here) then I suspect suspendPendingRequests/resumePendingRequests could be complete no-ops. But getting rid of NetworkProcess side suspend would be a good first step.
Or perhaps rather not be called.
As part of resolving <rdar://problem/12866005> I'll make those changes, and will file a new bugzilla for that tomorrow.
(In reply to comment #29) > As part of resolving <rdar://problem/12866005> I'll make those changes, and will file a new bugzilla for that tomorrow. https://bugs.webkit.org/show_bug.cgi?id=105550 filed for this.