Bug 53192

Summary: Add experimental support for HTTP pipelining in CFNetwork
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Page LoadingAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, dglazkov, koivisto, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 51890    
Bug Blocks: 53241    
Attachments:
Description Flags
Patch
koivisto: review+
Patch v2 [don't review--testing builds]
ddkilzer: commit-queue-
Patch v3 [don't review; testing builds] ddkilzer: review+, ddkilzer: commit-queue-

Description David Kilzer (:ddkilzer) 2011-01-26 14:22:04 PST
Created attachment 80237 [details]
Patch

Reviewed by NOBODY (OOPS!).

Source/WebCore:

This adds support for HTTP pipelining in CFNetwork, but does not
enable it.  To enable it post-SnowLeopard, use this command:

defaults write BUNDLE.ID WebKitEnableHTTPPipelining -bool YES

Once enabled, it is possible to force the same load priority
(high) to be sent to CFNetwork to allow WebCore to handle the
scheduling:

defaults write BUNDLE.ID WebKitForceHTTPPipeliningPriorityHigh -bool YES

* WebCore.exp.in: Export _wkGetHttpPipeliningPriority and
_wkSetHttpPipeliningPriority.

* loader/DocumentThreadableLoader.cpp:
(WebCore::DocumentThreadableLoader::makeCrossOriginAccessRequestWithPreflight):
Copy the priority to preflightRequest.

* loader/ResourceLoadScheduler.cpp:
(WebCore::ResourceLoadScheduler::scheduleLoad): If HTTP
pipelining is enabled, serve all requests regardless of priority
to keep the pipeline full.  Refactored code at the end of the
method to use an early return.

* loader/cache/CachedResourceRequest.cpp:
(WebCore::CachedResourceRequest::load): Set the priority on the
ResourceRequest object based on the priority of the
CachedResourceRequest before calling
ResourceLoadScheduler::scheduleSubresourceLoad().

* loader/icon/IconLoader.cpp:
(WebCore::IconLoader::startLoading): Create a ResourceRequest
object and set its priority to ResourceLoadPriorityLow before
passing it to ResourceLoadScheduler::scheduleSubresourceLoad().

* platform/mac/WebCoreSystemInterface.h:
(wkGetHttpPipeliningPriority): Added.
(wkSetHttpPipeliningPriority): Added.
* platform/mac/WebCoreSystemInterface.mm:
(wkGetHttpPipeliningPriority): Added.
(wkSetHttpPipeliningPriority): Added.

* platform/network/ResourceRequestBase.cpp:
(WebCore::ResourceRequestBase::adopt): Set m_priority when
adopting a CrossThreadResourceRequestData.
(WebCore::ResourceRequestBase::copyData): Set m_priority when
creating a CrossThreadResourceRequestData.
(WebCore::ResourceRequestBase::priority): Added.
(WebCore::ResourceRequestBase::setPriority): Added.
(WebCore::equalIgnoringHeaderFields): Priorities must match when
comparing two ResourceRequest objects.

* platform/network/ResourceRequestBase.h:
(WebCore::ResourceRequestBase::ResourceRequestBase): Set default
priority of new objects to ResourceLoadPriorityLow.
(WebCore::ResourceRequestBase::priority): Added declaration.
(WebCore::ResourceRequestBase::setPriority): Added declaration.
(WebCore::isHttpPipeliningEnabled): Added.
(WebCore::shouldUseHttpPipeliningPriority): Added.

* platform/network/cf/ResourceRequestCFNet.cpp: Updated so that
Mac OS X and Windows share code.
(WebCore::initializeMaximumHTTPConnectionCountPerHost): Always
set the HTTP connection count per host, but return an
'unlimited' value when using HTTP pipelining.  This method used
to be defined in ResourceRequestMac.mm for Mac OS X.
(WebCore::readBooleanPreference): Added.  Helper method for
reading boolean user defaults.
(WebCore::isHttpPipeliningEnabled): Returns value of user
default key WebKitEnableHTTPPipelining, or false if not set.
(WebCore::shouldUseHttpPipeliningPriority): Returns value of
user default key WebKitForceHTTPPipeliningPriorityHigh, or false
if not set.
* platform/network/cf/ResourceRequestCFNet.h: Updated so that
Mac OS X and Windows share code.  Fixed indentation.
(WebCore::mapHttpPipeliningPriorityToResourceLoadPriority): Added.
(WebCore::mapResourceLoadPriorityToHttpPipeliningPriority): Added.

* platform/network/mac/ResourceRequestMac.mm:
(WebCore::ResourceRequest::doUpdatePlatformRequest): Update
HTTP pipelining priority on NSMutableFURLRequest object.
(WebCore::ResourceRequest::doUpdateResourceRequest): Update
m_priority from the NSURLRequest object.
(WebCore::initializeMaximumHTTPConnectionCountPerHost): Removed.
Code is now shared with Windows in ResourceRequestCFNet.cpp.

Source/WebKit/mac:

* WebCoreSupport/WebSystemInterface.mm:
(InitWebCoreSystemInterface): Added initialization for
GetHttpPipeliningPriority and SetHttpPipeliningPriority.

Source/WebKit2:

* WebProcess/WebCoreSupport/mac/WebSystemInterface.mm:
(InitWebCoreSystemInterface): Added initialization for
GetHttpPipeliningPriority and SetHttpPipeliningPriority.

WebKitLibraries:

* WebKitSystemInterface.h:
New methods added for HTTP pipelining support.
(WKGetHttpPipeliningPriority): Added.
(WKSetHttpPipeliningPriority): Added.
Unrelated methods added after updating the header.
(WKMakeScrollbarPainter): Added.
(WKScrollbarPainterPaint): Added.
* libWebKitSystemInterfaceLeopard.a: Updated.
* libWebKitSystemInterfaceSnowLeopard.a: Updated.
---
 21 files changed, 302 insertions(+), 25 deletions(-)
Comment 1 WebKit Review Bot 2011-01-26 14:24:51 PST
Attachment 80237 [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/platform/network/ResourceRequestBase.h:133:  The parameter name "priority" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/ResourceRequestCFNet.h:47:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/platform/network/cf/ResourceRequestCFNet.h:62:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 David Kilzer (:ddkilzer) 2011-01-26 14:37:48 PST
(In reply to comment #1)
> Attachment 80237 [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/platform/network/ResourceRequestBase.h:133:  The parameter name "priority" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Source/WebCore/platform/network/cf/ResourceRequestCFNet.h:47:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
> Source/WebCore/platform/network/cf/ResourceRequestCFNet.h:62:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
> Total errors found: 3 in 19 files

I've fixed these issues locally.
Comment 3 WebKit Review Bot 2011-01-26 15:04:56 PST
Attachment 80237 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7535316
Comment 4 David Kilzer (:ddkilzer) 2011-01-26 15:17:54 PST
(In reply to comment #3)
> Attachment 80237 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/7535316

Not sure what to do here.  I don't see where Chromium is defining isHttpPipeliningEnabled() in the WebKit source tree.
Comment 5 David Kilzer (:ddkilzer) 2011-01-26 15:34:55 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Attachment 80237 [details] [details] did not build on chromium:
> > Build output: http://queues.webkit.org/results/7535316
> 
> Not sure what to do here.  I don't see where Chromium is defining isHttpPipeliningEnabled() in the WebKit source tree.

I guess I can change this in WebCore/platform/network/ResourceRequestBase.h:

+#if PLATFORM(CF) || PLATFORM(CHROMIUM)
+    bool isHttpPipeliningEnabled();
+    bool shouldForceHttpPipeliningPriorityHigh();
+#else
+    bool isHttpPipeliningEnabled() { return false; }
+    bool shouldForceHttpPipeliningPriorityHigh() { return false; }
+#endif
Comment 6 Antti Koivisto 2011-01-26 16:01:18 PST
Comment on attachment 80237 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80237&action=review

Looks good, r=me with build fixes + stylebot fixes + a comment below.

> Source/WebCore/loader/ResourceLoadScheduler.cpp:124
> +    if (isHttpPipeliningEnabled()) {
> +        // Serve all requests at once to keep the pipeline full at the network layer.
> +        servePendingRequests(host, ResourceLoadPriorityLowest);
> +        return;
> +    }

This change looks like an optimization rather than something that is needed for the patch to work. Did you benchmark it? It might actually hurt performance in some cases (by making us load images before more important resources). It would probably be better do it separately if it actually helps.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:116
>      ResourceLoadPriority priority = resource->loadPriority();
> +    resourceRequest.setPriority(priority);

It would be nicer if the ResourceLoadScheduler would set this. That would allow us to choose in one place how much networking layer level prioritization we want. But I see the request is const so this is fine for this patch.
Comment 7 Early Warning System Bot 2011-01-26 16:10:41 PST
Attachment 80237 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7565386
Comment 8 David Kilzer (:ddkilzer) 2011-01-26 16:34:13 PST
(In reply to comment #6)
> (From update of attachment 80237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80237&action=review
> 
> Looks good, r=me with build fixes + stylebot fixes + a comment below.

Okay, I'll post some more patches to try to get the builds fixed.

> > Source/WebCore/loader/ResourceLoadScheduler.cpp:124
> > +    if (isHttpPipeliningEnabled()) {
> > +        // Serve all requests at once to keep the pipeline full at the network layer.
> > +        servePendingRequests(host, ResourceLoadPriorityLowest);
> > +        return;
> > +    }
> 
> This change looks like an optimization rather than something that is needed for the patch to work. Did you benchmark it? It might actually hurt performance in some cases (by making us load images before more important resources). It would probably be better do it separately if it actually helps.

I did not benchmark it.  I'll remove it from the patch.

> > Source/WebCore/loader/cache/CachedResourceRequest.cpp:116
> >      ResourceLoadPriority priority = resource->loadPriority();
> > +    resourceRequest.setPriority(priority);
> 
> It would be nicer if the ResourceLoadScheduler would set this. That would allow us to choose in one place how much networking layer level prioritization we want. But I see the request is const so this is fine for this patch.

Okay.
Comment 9 David Kilzer (:ddkilzer) 2011-01-26 16:36:42 PST
(In reply to comment #7)
> Attachment 80237 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/7565386

Or I need to mark the other methods as explicitly inlined:

#if PLATFORM(CF)
    bool isHTTPPipeliningEnabled();
    bool shouldForceHTTPPipeliningPriorityHigh();
#else
    inline bool isHTTPPipeliningEnabled() { return false; }
    inline bool shouldForceHTTPPipeliningPriorityHigh() { return false; }
#endif
Comment 10 David Kilzer (:ddkilzer) 2011-01-26 16:58:54 PST
Created attachment 80268 [details]
Patch v2 [don't review--testing builds]
Comment 11 David Kilzer (:ddkilzer) 2011-01-26 17:12:51 PST
Created attachment 80271 [details]
Patch v3 [don't review; testing builds]
Comment 12 David Kilzer (:ddkilzer) 2011-01-26 20:48:13 PST
<rdar://problem/8821760>
Comment 13 David Kilzer (:ddkilzer) 2011-01-26 20:58:16 PST
Committed r76756.
http://trac.webkit.org/changeset/76756
Comment 14 David Kilzer (:ddkilzer) 2011-01-26 20:58:48 PST
Comment on attachment 80271 [details]
Patch v3 [don't review; testing builds]

This was the patch that was committed.  Marking as r+.