Make preconnectTo()'s completionHandler optional

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Make preconnectTo()'s completionHandler optional, so that the network process does not unnecessarily send an IPC back if the client is not interested.

[Chris Dumez]

Created attachment 385149 [details]
Patch

[Chris Dumez]

Created attachment 385189 [details]
Patch

[Darin Adler]

Comment on attachment 385149 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:191
> +    void preconnectTo(Optional<uint64_t>&& preconnectionIdentifier, NetworkResourceLoadParameters&&);

Seems like overkill to use an rvalue reference for an Optional. Just pass by value maybe?

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 385149 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385149&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:191
> > +    void preconnectTo(Optional<uint64_t>&& preconnectionIdentifier, NetworkResourceLoadParameters&&);
> 
> Seems like overkill to use an rvalue reference for an Optional. Just pass by
> value maybe?

I thought our policy was 'pass by value' only if <= 64 bit because it is faster. Do I remember wrong?

[Darin Adler]

Comment on attachment 385149 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:191
>>> +    void preconnectTo(Optional<uint64_t>&& preconnectionIdentifier, NetworkResourceLoadParameters&&);
>> 
>> Seems like overkill to use an rvalue reference for an Optional. Just pass by value maybe?
> 
> I thought our policy was 'pass by value' only if <= 64 bit because it is faster. Do I remember wrong?

Anders told me a long time ago that passing StringView by value was faster than by reference. I never questioned that or tested it.

[Chris Dumez]

Created attachment 385199 [details]
Patch

[Chris Dumez]

Comment on attachment 385199 [details]
Patch

Clearing flags on attachment: 385199

Committed r253325: <https://trac.webkit.org/changeset/253325>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57794165>

[Charlie Turner]

Comment on attachment 385199 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:669
> +    Optional<uint64_t> preconnectionIdentifier;

I'm not on a platform where I can test preconnection logic (!ENABLE(LEGACY_CUSTOM_PROTOCOL_MANAGER) && !ENABLE(SERVER_PRECONNECT)), but could we avoid the .send(...) at the bottom of this function if (!preconnectionIdentifier) ? I'm getting many internal errors thrown to stderr without extra information in NetworkConnectionToWebProcess::preconnectTo due to the internalError being called because of the network process side of preconnectTo being essentially a no-op on my platform. I could flag-off the call directly in preconnectTo, but reading the callsites it looked like maybe the IPC send could be avoided all-together in this case.

[Chris Dumez]

(In reply to Charlie Turner from comment #10)
> Comment on attachment 385199 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385199&action=review
> 
> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:669
> > +    Optional<uint64_t> preconnectionIdentifier;
> 
> I'm not on a platform where I can test preconnection logic
> (!ENABLE(LEGACY_CUSTOM_PROTOCOL_MANAGER) && !ENABLE(SERVER_PRECONNECT)), but
> could we avoid the .send(...) at the bottom of this function if
> (!preconnectionIdentifier) ? I'm getting many internal errors thrown to
> stderr without extra information in
> NetworkConnectionToWebProcess::preconnectTo due to the internalError being
> called because of the network process side of preconnectTo being essentially
> a no-op on my platform. I could flag-off the call directly in preconnectTo,
> but reading the callsites it looked like maybe the IPC send could be avoided
> all-together in this case.

If your port does not support preconnect, then yes, avoiding sending out the IPC is a good idea. If you write a patch for this, I'd be happy to review it. That said, it may not be a lot of work to add preconnect support for your port? :) Preconnect is a very useful optimization that WebKit is using more and more.

[Charlie Turner]

(In reply to Chris Dumez from comment #11)
> If your port does not support preconnect, then yes, avoiding sending out the
> IPC is a good idea. If you write a patch for this, I'd be happy to review
> it. That said, it may not be a lot of work to add preconnect support for
> your port? :) Preconnect is a very useful optimization that WebKit is using
> more and more.

I'll pass that on, thanks!

It turned out that this spurious error and the extra send() is already correctly avoided in trunk, being correctly conditioned out on the linkPreconnectEnabled web setting. In the WebKitGTK 2.28 release WebPage::loadRequest was calling preconnectTo outside of such a condition, which gave the errors. Sorry for not spotting master vs 2.28 difference earlier, but at least the question served a reminder we should implement preconnect hint.  :)