Bug 229686 – Preconnected socket is sometimes not used for initial request

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ben Nham]

Preconnected socket is sometimes not used for initial request

[Ben Nham]

Created attachment 436810 [details]
Patch

[Ben Nham]

<rdar://82461357>

[Ben Nham]

Created attachment 436843 [details]
Patch

[Ben Nham]

Created attachment 436846 [details]
Patch

[Ben Nham]

Created attachment 436961 [details]
Patch

[Ben Nham]

Created attachment 436977 [details]
Patch

[Ben Nham]

Created attachment 437527 [details]
Patch

[Alex Christensen]

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.

[Antti Koivisto]

This seems like a reasonable workaround to me.

[Ben Nham]

(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.

[Ben Nham]

Created attachment 437634 [details]
Patch

[Alex Christensen]

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.

[Ben Nham]

(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.

[Ben Nham]

Created attachment 437645 [details]
Patch

[Alex Christensen]

Comment on attachment 437645 [details]
Patch

I think we should still push CFNetwork to fix this and remove this code once they do.

[Ben Nham]

(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.

[EWS]

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].