NetworkLoadMetrics should be shared by multiple ResourceResponse instances
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>