Bug 207747

Created attachment 390732 [details]
Patch

Comment on attachment 390732 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        has Vector<ResourceResponse> to replay response dispatching in the case of loading from BackForwardCache. The problem is

Note that this is one of the largest Vector in Membuster.

<rdar://problem/59452102>

Created attachment 390737 [details]
Patch

Created attachment 390738 [details]
Patch

Created attachment 390739 [details]
Patch

Created attachment 390740 [details]
Patch

Created attachment 390741 [details]
Patch

Created attachment 390743 [details]
Patch

I'm also considering that making ResourceResponse RefCounted<>, but as a first step, I'll do NetworkLoadMetrics part.

I think we could make ResourceResponse RefCounted<> without changing much of things.
Due to WebKit2, we already initialize all the fields of ResourceResponse anyway.
So, we could make almost all part as immutable.

Created attachment 390755 [details]
Patch

Comment on attachment 390755 [details]
Patch

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

r=me with nits.

> Source/WebCore/ChangeLog:13
> +        While we sometimes copies it to modify some part of it, NetworkLoadMetrics is immutable. It is set when response is created,

nit: copies => copy

Also, what is "it" in "copies it"? A NeworkLoadMetrics or a ResourceResponse?

> Source/WebCore/ChangeLog:17
> +        This patch puts Box<NetworkLoadMetrics> in ResourceResponse to share it by all copied ResourceResponses. We do not make NetworkLoadMetrics

Nit: share it by => hare it with.

> Source/WebCore/ChangeLog:18
> +        RefCounted<> for now since some legit data structure embeds NetworkLoadMetrics. This patch adds ArgumentCoder for Box so that we

Nit: data structure embeds => data structures embed

Or did you mean that there is only on data structure that embed's NetworkLoadMetrics? If so can you name that class?

> Source/WebCore/platform/network/ResourceResponseBase.h:339
> +    if constexpr (Decoder::isIPCDecoder) {
> +        if (!decoder.decode(response.m_networkLoadMetrics))
> +            return false;
> +    }

Why bother with this change? Seems like the compiler will make this optimization anyway?

> Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:68
> +

Nit: remove newline here.

Comment on attachment 390755 [details]
Patch

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

Thanks!

>> Source/WebCore/ChangeLog:13
>> +        While we sometimes copies it to modify some part of it, NetworkLoadMetrics is immutable. It is set when response is created,
> 
> nit: copies => copy
> 
> Also, what is "it" in "copies it"? A NeworkLoadMetrics or a ResourceResponse?

Fixed. DocumentLoader for example is copying ResourceResponse.

>> Source/WebCore/ChangeLog:17
>> +        This patch puts Box<NetworkLoadMetrics> in ResourceResponse to share it by all copied ResourceResponses. We do not make NetworkLoadMetrics
> 
> Nit: share it by => hare it with.

Fixed.

>> Source/WebCore/ChangeLog:18
>> +        RefCounted<> for now since some legit data structure embeds NetworkLoadMetrics. This patch adds ArgumentCoder for Box so that we
> 
> Nit: data structure embeds => data structures embed
> 
> Or did you mean that there is only on data structure that embed's NetworkLoadMetrics? If so can you name that class?

Fixed. Some of multiple classes are embedding NetworkLoadMetrics. So for now, I'm taking an easy path.

>> Source/WebCore/platform/network/ResourceResponseBase.h:339
>> +    }
> 
> Why bother with this change? Seems like the compiler will make this optimization anyway?

Encoder can be persistent coder (which generates bytes for disks).
We can omit implementation of persistent coder's NetworkLoadMetrics handling if we use `if constexpr`.

>> Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:68
>> +
> 
> Nit: remove newline here.

Fixed.

Committed r256632: <https://trac.webkit.org/changeset/256632>