Bug 158232

Summary: Reduce ResourceResponse copying
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, berto, cdumez, cgarcia, cmarcelo, commit-queue, danw, gns, japhet, mcatanzaro, mrobinson, pnormand
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alex Christensen 2016-05-31 13:39:53 PDT
Reduce ResourceResponse copying
Comment 1 Alex Christensen 2016-05-31 13:43:42 PDT
Created attachment 280173 [details]
Patch
Comment 2 Chris Dumez 2016-05-31 13:48:03 PDT
Comment on attachment 280173 [details]
Patch

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

> Source/WTF/wtf/Optional.h:184
> +    template<class Encoder> void encode(Encoder&) const;

What's wrong with the version that is already in ArgumentCoders.h?
Comment 3 Chris Dumez 2016-05-31 13:51:04 PDT
Comment on attachment 280173 [details]
Patch

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

> Source/WebCore/platform/network/ResourceResponseBase.cpp:184
>      m_certificateInfo = static_cast<const ResourceResponse*>(this)->platformCertificateInfo();

Presumably we no longer need the const in the cast here.

> Source/WebCore/platform/network/ResourceResponseBase.h:176
> +#define RESOURCE_RESPONSE_BASE_CERT_INFO_OPTIONAL

What's this?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:471
> +    webPage->send(Messages::WebPageProxy::DidCommitLoadForFrame(m_frame->frameID(), documentLoader.navigationID(), documentLoader.response().mimeType(), m_frameHasCustomContentProvider, static_cast<uint32_t>(m_frame->coreFrame()->loader().loadType()), documentLoader.response().certificateInfo().valueOr(CertificateInfo()), m_frame->coreFrame()->document()->isPluginDocument(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));

Is constructing a CertificateInfo() cheap enough that we want to do it every time?

> Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409
> +    return documentLoader->response().certificateInfo().valueOr(CertificateInfo());

Is constructing a CertificateInfo() cheap enough that we want to do it every time?
Comment 4 Chris Dumez 2016-05-31 13:51:32 PDT
Comment on attachment 280173 [details]
Patch

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

> Source/WebKit2/UIProcess/Authentication/AuthenticationChallengeProxy.cpp:74
> +    if (credential->certificateInfo() && credential->certificateInfo())

LoL :)
Comment 5 Alex Christensen 2016-05-31 14:07:11 PDT
Created attachment 280181 [details]
Patch
Comment 6 WebKit Commit Bot 2016-05-31 14:09:42 PDT
Attachment 280181 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Alex Christensen 2016-05-31 14:12:17 PDT
Comment on attachment 280181 [details]
Patch

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

> Source/WebCore/platform/network/ResourceResponseBase.h:176
> +#define RESOURCE_RESPONSE_BASE_CERT_INFO_OPTIONAL

This is temporary and to assist a change in another repository.
Comment 8 Alex Christensen 2016-05-31 15:35:34 PDT
Comment on attachment 280181 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.h:30
> +#include "ArgumentCoders.h"

This isn't needed
Comment 9 Alex Christensen 2016-06-06 22:33:09 PDT
Created attachment 280670 [details]
Patch
Comment 10 WebKit Commit Bot 2016-06-06 22:34:20 PDT
Attachment 280670 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Darin Adler 2016-06-09 20:29:19 PDT
Comment on attachment 280670 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        (WebCore::ResourceResponseBase::includeCertificateInfo): Made non-const because it modifies the ResourceResponse.

I am not sure what your rationale was for changing this. The person who did the original work intentionally used const and mutable. Could you give the rationale?

> Source/WebCore/loader/SubresourceLoader.cpp:244
>      if (shouldIncludeCertificateInfo())
> -        response.includeCertificateInfo();
> +        const_cast<ResourceResponse&>(response).includeCertificateInfo(); // FIXME: response should not be const.

Why not fix this instead of adding the const_cast?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:194
> +Optional<CertificateInfo> ResourceResponseBase::certificateInfo() const
>  {
>      return m_certificateInfo;
>  }

I suggest we make this an inline in the header.

> Source/WebCore/platform/network/ResourceResponseBase.h:117
> +    WEBCORE_EXPORT Optional<CertificateInfo> certificateInfo() const;

For better efficiency, I suggest the return type const Optional<CertificateInfo>&. Otherwise, calling this will copy the certificate info. And so it will be much less efficient than calling containsCertificateInfo() was.

> Source/WebCore/platform/network/ResourceResponseBase.h:192
> +#define RESOURCE_RESPONSE_BASE_CERT_INFO_OPTIONAL

What is this for?

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:293
> +    // FIXME: receivedResponse should not be const.
> +    if (sharedDidReceiveResponse(const_cast<ResourceResponse&>(receivedResponse)) == NetworkLoadClient::ShouldContinueDidReceiveResponse::Yes)
>          m_handle->continueDidReceiveResponse();

Why a FIXME instead of fixing it?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.h:84
> +template<typename T> struct Coder<WTF::Optional<T>> {
> +    static void encode(Encoder& encoder, const WTF::Optional<T>& optional)

Can’t this just be Optional, rather than WTF::Optional?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.h:95
> +    static bool decode(Decoder& decoder, WTF::Optional<T>& optional)

Can’t this just be Optional, rather than WTF::Optional?
Comment 12 Alex Christensen 2016-06-10 11:24:54 PDT
Comment on attachment 280670 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        (WebCore::ResourceResponseBase::includeCertificateInfo): Made non-const because it modifies the ResourceResponse.
> 
> I am not sure what your rationale was for changing this. The person who did the original work intentionally used const and mutable. Could you give the rationale?

You're right.  This is not a good change.  It just copies things from the platform data to the ResourceResponseBase, which doesn't actually change the ResourceResponse conceptually.

>> Source/WebCore/platform/network/ResourceResponseBase.h:192
>> +#define RESOURCE_RESPONSE_BASE_CERT_INFO_OPTIONAL
> 
> What is this for?

This is so we don't break the internal build when I land this, because certificateInfo() is called from WebKitAdditions.  It's temporary and will be removed.
Comment 13 Alex Christensen 2016-06-10 12:27:08 PDT
Created attachment 281031 [details]
Patch
Comment 14 WebKit Commit Bot 2016-06-10 12:28:49 PDT
Attachment 281031 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Alex Christensen 2016-06-10 13:05:04 PDT
Created attachment 281036 [details]
Patch
Comment 16 WebKit Commit Bot 2016-06-10 13:06:11 PDT
Attachment 281036 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Alex Christensen 2016-06-10 13:15:28 PDT
Created attachment 281038 [details]
Patch
Comment 18 WebKit Commit Bot 2016-06-10 13:17:37 PDT
Attachment 281038 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Alex Christensen 2016-06-10 13:27:59 PDT
Created attachment 281041 [details]
Patch
Comment 20 WebKit Commit Bot 2016-06-10 13:29:22 PDT
Attachment 281041 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Alex Christensen 2016-06-10 14:17:07 PDT
Created attachment 281046 [details]
Patch
Comment 22 WebKit Commit Bot 2016-06-10 14:18:53 PDT
Attachment 281046 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Alex Christensen 2016-06-10 15:02:25 PDT
http://trac.webkit.org/changeset/201943
Comment 24 Philippe Normand 2016-07-15 07:36:41 PDT
Comment on attachment 281046 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1095
>      handleResponseReceived(response);

Shouldn't this use WTFMove(response) ?
Comment 25 Chris Dumez 2016-07-15 08:24:08 PDT
Comment on attachment 281046 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1095
>>      handleResponseReceived(response);
> 
> Shouldn't this use WTFMove(response) ?

It could but it would not help unless handleResponseReceived() is also updated to take a ResourceResponse&&.