Bug 203423 - VeryHigh priority loads are actually loading at VeryLow priority
Summary: VeryHigh priority loads are actually loading at VeryLow priority
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Nham
Keywords: InRadar
Depends on: 203594
  Show dependency treegraph
Reported: 2019-10-25 10:02 PDT by Ben Nham
Modified: 2019-11-13 14:41 PST (History)
6 users (show)

See Also:

Patch (5.40 KB, patch)
2019-10-29 15:59 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (8.04 KB, patch)
2019-10-29 16:25 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (8.10 KB, patch)
2019-11-13 12:28 PST, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (8.14 KB, patch)
2019-11-13 13:10 PST, Ben Nham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Nham 2019-10-25 10:02:50 PDT
We tag our resources with one out of 5 priority levels, from ResourceLoadPriority::VeryLow to ResourceLoadPriority::VeryHigh. On Mac and iOS, we translate these levels into one of five CFURLRequestPriority priority levels from 0 (VeryLow) to 4 (VeryHigh) inclusive. CFNetwork then uses these priorities to prioritize which requests should go out first on a connection if there are multiple queued requests. This effect is most pronounced for HTTP/1.1 where it is common to saturate the six-connections-per-host rule, and there are often multiple requests queued up waiting for a connection to free up.

The problem is that we actually tell CFNetwork that we only want 4 priority levels in various places. For instance, in NetworkProcessCocoa::initializeNetworkSettings:

  // toPlatformRequestPriority(WebCore::ResourceLoadPriority::Highest) = 4, so this only gives us 4 priorities not 5!
  _CFNetworkHTTPConnectionCacheSetLimit(kHTTPPriorityNumLevels, toPlatformRequestPriority(WebCore::ResourceLoadPriority::Highest)); 

When CFNetwork detects an out-of-bounds priority level, it resets the priority of the request down to 0 (VeryLow). So this means our VeryHigh priority requests are actually being treated as VeryLow priority requests.

For WebKit2, there is actually a second issue: even if we didn't have this off-by-one error in the call to _CFNetworkHTTPConnectionCacheSetLimit, we'd still be in trouble because CFNetwork doesn't actually apply the connection cache limits from _CFNetworkHTTPConnectionCacheSetLimit to default or ephemeral NSURLSessionConfigurations, which is what we use in NetworkProcess (<rdar://problem/56621205>).

Note that this bug is about CFURLRequestPriority, not NSURLSessionTaskPriority. The latter doesn't do anything for HTTP/1.1 is mainly used to set the stream weight/priority field for HTTP2 connections.
Comment 1 Radar WebKit Bug Importer 2019-10-25 10:03:46 PDT
Comment 2 Antti Koivisto 2019-10-25 10:46:19 PDT
Comment 3 Antti Koivisto 2019-10-25 10:47:38 PDT
Nice find!
Comment 4 Ben Nham 2019-10-29 15:59:26 PDT
Created attachment 382243 [details]
Comment 5 Ben Nham 2019-10-29 16:25:22 PDT
Created attachment 382249 [details]
Comment 6 Ben Nham 2019-10-29 19:01:26 PDT
The attached patch shows a consistent 20-25% improvement for cold loading of nytimes-article in PLT5 on the perf bots, which is exactly what I expected since that is the main resource load most affected by connection congestion in the test. Because we load 13 websites in PLT5, this shows up as a 1.5-2.25% cold PLT5 improvement.

This shows up as a neutral change in overall PLT5 because:

1. We weight warm/cold loads in PLT5 80:20.
2. Some of the resource load timings changed enough in the warm loads that we ended up capturing more JS before deciding on first paint, which slightly regressed warm PLT5 (by ~1%) in certain configurations. However, I don't think we should hold off on making this change since I'm planning to do a pass on first paint timings soon.
Comment 7 Antti Koivisto 2019-10-30 02:04:05 PDT
Comment on attachment 382249 [details]

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

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.h:43
> +    // TODO(nham): switch VeryLow back to 0 priority when CFNetwork fixes <rdar://problem/56621205>

We usually just say FIXME: without a name. Anyone is free to fix problems and if the FIXME originator is important it can be found with blame.
Comment 8 Ben Nham 2019-11-13 12:28:58 PST
Created attachment 383480 [details]
Comment 9 Ben Nham 2019-11-13 12:30:46 PST
Rebased and replaced TODO with FIXME.
Comment 10 WebKit Commit Bot 2019-11-13 12:40:10 PST
Comment on attachment 383480 [details]

Rejecting attachment 383480 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 383480, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/13248130
Comment 11 Ben Nham 2019-11-13 13:10:56 PST
Created attachment 383484 [details]
Comment 12 Ben Nham 2019-11-13 13:11:32 PST
Comment on attachment 383484 [details]

Add reviewed-by line to WebKit ChangeLog.
Comment 13 WebKit Commit Bot 2019-11-13 14:41:15 PST
Comment on attachment 383484 [details]

Clearing flags on attachment: 383484

Committed r252431: <https://trac.webkit.org/changeset/252431>
Comment 14 WebKit Commit Bot 2019-11-13 14:41:17 PST
All reviewed patches have been landed.  Closing bug.