Bug 229686 - Preconnected socket is sometimes not used for initial request
Summary: Preconnected socket is sometimes not used for initial request
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Nham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-30 13:59 PDT by Ben Nham
Modified: 2021-09-09 17:28 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.31 KB, patch)
2021-08-30 14:00 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (24.74 KB, patch)
2021-08-30 21:26 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (25.43 KB, patch)
2021-08-30 23:08 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (27.92 KB, patch)
2021-08-31 16:18 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (28.61 KB, patch)
2021-08-31 18:10 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (30.14 KB, patch)
2021-09-07 10:30 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (29.89 KB, patch)
2021-09-08 09:13 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (15.70 KB, patch)
2021-09-08 10:21 PDT, 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 2021-08-30 13:59:11 PDT
Preconnected socket is sometimes not used for initial request
Comment 1 Ben Nham 2021-08-30 14:00:57 PDT
Created attachment 436810 [details]
Patch
Comment 2 Ben Nham 2021-08-30 14:01:46 PDT
<rdar://82461357>
Comment 3 Ben Nham 2021-08-30 21:26:21 PDT
Created attachment 436843 [details]
Patch
Comment 4 Ben Nham 2021-08-30 23:08:52 PDT
Created attachment 436846 [details]
Patch
Comment 5 Ben Nham 2021-08-31 16:18:03 PDT
Created attachment 436961 [details]
Patch
Comment 6 Ben Nham 2021-08-31 18:10:46 PDT
Created attachment 436977 [details]
Patch
Comment 7 Ben Nham 2021-09-07 10:30:55 PDT
Created attachment 437527 [details]
Patch
Comment 8 Alex Christensen 2021-09-07 10:43:55 PDT
Comment on attachment 437527 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadScheduler.cpp:196
> +void NetworkLoadScheduler::startedPreconnectForMainResource(const URL& url)

This seems like a lot of logic for scheduling http1 preloads sequentially.  I would think this logic should go in CFNetwork checking if it has a preconnect task in flight instead of WebKit implementing its own scheduler.

> Source/WebKit/NetworkProcess/NetworkLoadScheduler.h:74
> +        ListHashSet<NetworkLoad *> pendingLoads;

We shouldn't be storing raw pointers like this.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1397
> +    if (reason == PreconnectReason::MainResourceLoad) {
> +        parameters.request.setPriority(WebCore::ResourceLoadPriority::VeryHigh);

Is there a reason we don't want to do this for preconnects from the API?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1405
> +    task->setTimeout(1_s);

This seems too short to make preconnects useful in the real internet.

> Source/WebKit/NetworkProcess/PreconnectReason.h:33
> +enum class PreconnectReason : uint8_t {

: bool
Then you don't need EnumTraits below because it already knows 0 and 1 are the only valid values.

> Source/WebKit/NetworkProcess/PreconnectTask.cpp:-50
> -    m_timeoutTimer.startOneShot(60000_s);

This was definitely unnecessarily long.

> Source/WebKit/NetworkProcess/PreconnectTask.h:32
> +#include <optional>

This looks unused.
Comment 9 Antti Koivisto 2021-09-08 00:44:44 PDT
This seems like a reasonable workaround to me.
Comment 10 Ben Nham 2021-09-08 08:36:29 PDT
(In reply to Alex Christensen from comment #8)
> Comment on attachment 437527 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437527&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkLoadScheduler.cpp:196
> > +void NetworkLoadScheduler::startedPreconnectForMainResource(const URL& url)
> 
> This seems like a lot of logic for scheduling http1 preloads sequentially. 
> I would think this logic should go in CFNetwork checking if it has a
> preconnect task in flight instead of WebKit implementing its own scheduler.

I agree that it should be implemented at that level, but their estimate is that they'd need a couple weeks of effort to implement our desired behavior. They actually tried to implement it in a branch so the effort estimate should be grounded in reality. I can reach out to you with more details.

The same argument could be applied to the rest of NetworkLoadScheduler, which already is delaying loads for other reasons and is also arguably better implemented further down the stack.

> > Source/WebKit/NetworkProcess/NetworkLoadScheduler.h:74
> > +        ListHashSet<NetworkLoad *> pendingLoads;
> 
> We shouldn't be storing raw pointers like this.

We are doing this already in other parts of NetworkLoadScheduler:

  class NetworkLoadScheduler::HostContext {
      // ...

      HashSet<NetworkLoad*> m_activeLoads;
      ListHashSet<NetworkLoad*> m_pendingLoads;
  };

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1397
> > +    if (reason == PreconnectReason::MainResourceLoad) {
> > +        parameters.request.setPriority(WebCore::ResourceLoadPriority::VeryHigh);
> 
> Is there a reason we don't want to do this for preconnects from the API?

If we don't set the priority here, then the preconnect ends up at the default priority (ResourceLoadPriority::Low). The eventual main resource request has a priority of ResourceLoadPriority::VeryHigh. This can cause a priority inversion as the hi-pri main resource ends up blocked on a low-pri preconnect which itself is blocked on some low-pri existing requests to the same origin to complete (since there is a limit on number of outstanding low-pri requests). This showed up in the page load benchmarks.

I chose to not bump the priority of API preconnects since the API doesn't tell us whether the preconnect is for a main resource or not. It only makes sense to bump the priority if the preconnect will eventually be used for a main resource.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1405
> > +    task->setTimeout(1_s);
> 
> This seems too short to make preconnects useful in the real internet.

I'll bump this up by some amount, at least large enough to cover a typical worst case for 3G when the radio needs to go through the full handshake for connection establishment at the link layer.

> > Source/WebKit/NetworkProcess/PreconnectReason.h:33
> > +enum class PreconnectReason : uint8_t {
> 
> : bool
> Then you don't need EnumTraits below because it already knows 0 and 1 are
> the only valid values.

I made a uint8_t because it doesn't logically seem to be a bool and there might eventually be more than two preconnect reasons. But I can change it to a bool for now and leave the change to a uint8 for a patch down the road.

> > Source/WebKit/NetworkProcess/PreconnectTask.cpp:-50
> > -    m_timeoutTimer.startOneShot(60000_s);
> 
> This was definitely unnecessarily long.
> 
> > Source/WebKit/NetworkProcess/PreconnectTask.h:32
> > +#include <optional>
> 
> This looks unused.

I'll remove this.
Comment 11 Ben Nham 2021-09-08 09:13:08 PDT
Created attachment 437634 [details]
Patch
Comment 12 Alex Christensen 2021-09-08 09:13:54 PDT
I think we should treat the API to preconnect as high priority too.  It does indicate that there is a main resource request to that domain coming, and making preconnect requests high priority doesn't actually use many resources which would delay other things.
Comment 13 Ben Nham 2021-09-08 09:54:32 PDT
(In reply to Alex Christensen from comment #12)
> I think we should treat the API to preconnect as high priority too.  It does
> indicate that there is a main resource request to that domain coming, and
> making preconnect requests high priority doesn't actually use many resources
> which would delay other things.

Ok, in that case I'll just remove PreconnectReason entirely.
Comment 14 Ben Nham 2021-09-08 10:21:13 PDT
Created attachment 437645 [details]
Patch
Comment 15 Alex Christensen 2021-09-09 15:56:01 PDT
Comment on attachment 437645 [details]
Patch

I think we should still push CFNetwork to fix this and remove this code once they do.
Comment 16 Ben Nham 2021-09-09 16:55:25 PDT
(In reply to Alex Christensen from comment #15)
> Comment on attachment 437645 [details]
> Patch
> 
> I think we should still push CFNetwork to fix this and remove this code once
> they do.

I definitely agree and added you to a thread where we're discussing that.
Comment 17 EWS 2021-09-09 17:28:47 PDT
Committed r282252 (241530@main): <https://commits.webkit.org/241530@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437645 [details].