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(-)
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.
(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.
Attachment 80237 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7535316
(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.
(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 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.
Attachment 80237 [details] did not build on qt: Build output: http://queues.webkit.org/results/7565386
(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.
(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
Created attachment 80268 [details] Patch v2 [don't review--testing builds]
Created attachment 80271 [details] Patch v3 [don't review; testing builds]
<rdar://problem/8821760>
Committed r76756. http://trac.webkit.org/changeset/76756
Comment on attachment 80271 [details] Patch v3 [don't review; testing builds] This was the patch that was committed. Marking as r+.