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!
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.
Created attachment 382243 [details]
Created attachment 382249 [details]
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 on attachment 382249 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=382249&action=review
> + // 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.
Created attachment 383480 [details]
Rebased and replaced TODO with FIXME.
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
Created attachment 383484 [details]
Comment on attachment 383484 [details]
Add reviewed-by line to WebKit ChangeLog.
Comment on attachment 383484 [details]
Clearing flags on attachment: 383484
Committed r252431: <https://trac.webkit.org/changeset/252431>
All reviewed patches have been landed. Closing bug.