Bug 100479 – Have NetworkProcess manage resource load scheduling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

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.

[Brady Eidson]

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.

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 170852 [details]
WIP patch

Attachment 170852 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14552958

[Brady Eidson]

Created attachment 170952 [details]
WIP #2 - Check style and mac build while I start in on changelogs

[WebKit Review Bot]

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.

[Brady Eidson]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Brady Eidson]

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

[Adam Barth]

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

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

Comment on attachment 170963 [details]
Patch v1

These comments are easy to address (mostly with FIXMEs), r=me.

[Brady Eidson]

(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  :)

[Brady Eidson]

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

[Brady Eidson]

http://trac.webkit.org/changeset/132721

[Simon Fraser (smfr)]

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

[Brady Eidson]

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

[Adam Barth]

You can run them with ./Tools/Scripts/run-perf-tests.  It looks they started crashing.

[Alexey Proskuryakov]

I think that bug 100602 took care of those crashes.

[Brady Eidson]

(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

[Brady Eidson]

(In reply to comment #19)
> I think that bug 100602 took care of those crashes.

Yah, that.  hehe

[Antti Koivisto]

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?

[Sam Weinig]

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

[Brady Eidson]

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

[Antti Koivisto]

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.

[Brady Eidson]

(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?

[Antti Koivisto]

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

[Antti Koivisto]

Or perhaps rather not be called.

[Brady Eidson]

As part of resolving <rdar://problem/12866005> I'll make those changes, and will file a new bugzilla for that tomorrow.

[Brady Eidson]

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