Bug 207747 – NetworkLoadMetrics should be shared by multiple ResourceResponse instances

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

[Yusuke Suzuki]

NetworkLoadMetrics should be shared by multiple ResourceResponse instances

[Yusuke Suzuki]

Created attachment 390732 [details]
Patch

[Yusuke Suzuki]

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.

[Radar WebKit Bug Importer]

<rdar://problem/59452102>

[Yusuke Suzuki]

Created attachment 390737 [details]
Patch

[Yusuke Suzuki]

Created attachment 390738 [details]
Patch

[Yusuke Suzuki]

Created attachment 390739 [details]
Patch

[Yusuke Suzuki]

Created attachment 390740 [details]
Patch

[Yusuke Suzuki]

Created attachment 390741 [details]
Patch

[Yusuke Suzuki]

Created attachment 390743 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 390755 [details]
Patch

[Keith Miller]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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