Bug 237055 – Preconnecting after process swap is a page load time improvement on some devices

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Preconnecting after process swap is a page load time improvement on some devices, but a regression on others.

[Per Arne Vollan]

Created attachment 452900 [details]
Patch

[Per Arne Vollan]

Created attachment 453002 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/89638872>

[Per Arne Vollan]

Created attachment 453755 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 453755 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        a second preconnect after the first has finished. It is important to wait until the first preconnect has
> +        finished, otherwise the second preconnect will go to waste, since the underlying network layer does not yet
> +        know if this is HTTP/1.1 or not.

In the case of libsoup, if there's already a connection in idle state preconnect does nothing. So, this patch isn't useful for soup based ports, but it's harmless anyway. Maybe it's worth adding platform ifdefs, though.

[Per Arne Vollan]

(In reply to Carlos Garcia Campos from comment #5)
> Comment on attachment 453755 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453755&action=review
> 
> > Source/WebKit/ChangeLog:13
> > +        a second preconnect after the first has finished. It is important to wait until the first preconnect has
> > +        finished, otherwise the second preconnect will go to waste, since the underlying network layer does not yet
> > +        know if this is HTTP/1.1 or not.
> 
> In the case of libsoup, if there's already a connection in idle state
> preconnect does nothing. So, this patch isn't useful for soup based ports,
> but it's harmless anyway. Maybe it's worth adding platform ifdefs, though.

Ah, yes, that is a good point. I will update the patch.

Thanks for reviewing!

[Geoffrey Garen]

> > In the case of libsoup, if there's already a connection in idle state
> > preconnect does nothing. So, this patch isn't useful for soup based ports,
> > but it's harmless anyway. Maybe it's worth adding platform ifdefs, though.
> 
> Ah, yes, that is a good point. I will update the patch.

Just a second: My understanding is that CFNetwork's behavior matches libsoup's behavior. But that doesn't invalidate the optimization.

Our thesis is that the prior http/1.1 connection is usually not idle at the time of this preconnect. It is busy loading the main resource request. So, the expected effect of this preconnect is to "go wide" and open a second http/1.1 connection in parallel to downloading the main resource request on the first connection. Then we get a speedup because the second connection is ready to go with zero latency the moment we parse the first subresource request.

[Geoffrey Garen]

Comment on attachment 453755 [details]
Patch

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

> Source/WebKit/NetworkProcess/PreconnectTask.cpp:45
> +, m_timeoutTimer([this] { didFinish(ResourceError { String(), 0, m_networkLoad->parameters().request.url(), "Preconnection timed out"_s, ResourceError::Type::Timeout }, { }); })

Looks like the indentation got messed up here.

[Geoffrey Garen]

Perhaps a more principled way to write this optimization would explicitly arrange not to make the second preconnect request until the main resource request has been made (thereby guaranteeing that the first connection is not idle).

Maybe we should test a change like that to see if it improves the optimization across more CPUs?

I guess a way to do that would be for the UI process to initiate the first preconnect right when the navigation starts (as it does today), and then initiate the second  preconnect right after we issue the load request for the main resource. Not sure exactly how to pipe the information that the connection is http/1.1 in this proposal -- maybe Per can figure out that piece.

[Carlos Garcia Campos]

(In reply to Geoffrey Garen from comment #7)
> > > In the case of libsoup, if there's already a connection in idle state
> > > preconnect does nothing. So, this patch isn't useful for soup based ports,
> > > but it's harmless anyway. Maybe it's worth adding platform ifdefs, though.
> > 
> > Ah, yes, that is a good point. I will update the patch.
> 
> Just a second: My understanding is that CFNetwork's behavior matches
> libsoup's behavior. But that doesn't invalidate the optimization.
> 
> Our thesis is that the prior http/1.1 connection is usually not idle at the
> time of this preconnect. It is busy loading the main resource request. So,
> the expected effect of this preconnect is to "go wide" and open a second
> http/1.1 connection in parallel to downloading the main resource request on
> the first connection. Then we get a speedup because the second connection is
> ready to go with zero latency the moment we parse the first subresource
> request.

The second preconnect is started right after the first one finishes right? I think it will be still idle at that point.

[Per Arne Vollan]

Created attachment 454003 [details]
Patch

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #8)
> Comment on attachment 453755 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453755&action=review
> 
> > Source/WebKit/NetworkProcess/PreconnectTask.cpp:45
> > +, m_timeoutTimer([this] { didFinish(ResourceError { String(), 0, m_networkLoad->parameters().request.url(), "Preconnection timed out"_s, ResourceError::Type::Timeout }, { }); })
> 
> Looks like the indentation got messed up here.

Fixed in the latest patch.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #9)
> Perhaps a more principled way to write this optimization would explicitly
> arrange not to make the second preconnect request until the main resource
> request has been made (thereby guaranteeing that the first connection is not
> idle).
> 
> Maybe we should test a change like that to see if it improves the
> optimization across more CPUs?
> 
> I guess a way to do that would be for the UI process to initiate the first
> preconnect right when the navigation starts (as it does today), and then
> initiate the second  preconnect right after we issue the load request for
> the main resource. Not sure exactly how to pipe the information that the
> connection is http/1.1 in this proposal -- maybe Per can figure out that
> piece.

Yes, this is a great idea. I think it would make sure that the second preconnect never goes to waste. I have started looking into this.

Would you be OK with landing the current patch as-is, and then follow up with a new patch? The current patch seems to be a nice progression on macOS, at least on the devices that have been tested.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Carlos Garcia Campos from comment #10)
> (In reply to Geoffrey Garen from comment #7)
> > > > In the case of libsoup, if there's already a connection in idle state
> > > > preconnect does nothing. So, this patch isn't useful for soup based ports,
> > > > but it's harmless anyway. Maybe it's worth adding platform ifdefs, though.
> > > 
> > > Ah, yes, that is a good point. I will update the patch.
> > 
> > Just a second: My understanding is that CFNetwork's behavior matches
> > libsoup's behavior. But that doesn't invalidate the optimization.
> > 
> > Our thesis is that the prior http/1.1 connection is usually not idle at the
> > time of this preconnect. It is busy loading the main resource request. So,
> > the expected effect of this preconnect is to "go wide" and open a second
> > http/1.1 connection in parallel to downloading the main resource request on
> > the first connection. Then we get a speedup because the second connection is
> > ready to go with zero latency the moment we parse the first subresource
> > request.
> 
> The second preconnect is started right after the first one finishes right? I
> think it will be still idle at that point.

In the latest patch, finishedPreconnectForMainResource is called before starting the second preconnect. Calling finishedPreconnectForMainResource will in some cases start the load.

Thanks for reviewing!

[Geoffrey Garen]

Comment on attachment 454003 [details]
Patch

r=me

[Geoffrey Garen]

> Would you be OK with landing the current patch as-is, and then follow up
> with a new patch?

Yup!

[EWS]

Committed r290969 (248148@main): <https://commits.webkit.org/248148@main>

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