VeryHigh priority loads are actually loading at VeryLow priority

[Ben Nham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/56621789>

[Antti Koivisto]

Lol

[Antti Koivisto]

Nice find!

[Ben Nham]

Created attachment 382243 [details]
Patch

[Ben Nham]

Created attachment 382249 [details]
Patch

[Ben Nham]

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.

[Antti Koivisto]

Comment on attachment 382249 [details]
Patch

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.

[Ben Nham]

Created attachment 383480 [details]
Patch

[Ben Nham]

Rebased and replaced TODO with FIXME.

[WebKit Commit Bot]

Comment on attachment 383480 [details]
Patch

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

[Ben Nham]

Created attachment 383484 [details]
Patch

[Ben Nham]

Comment on attachment 383484 [details]
Patch

Add reviewed-by line to WebKit ChangeLog.

[WebKit Commit Bot]

Comment on attachment 383484 [details]
Patch

Clearing flags on attachment: 383484

Committed r252431: <https://trac.webkit.org/changeset/252431>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.