Reduce size of NetworkLoadMetrics and therefore ResourceResponse

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Reduce size of NetworkLoadMetrics and therefore ResourceResponse

[Alex Christensen]

Created attachment 345001 [details]
Patch

[Alex Christensen]

Created attachment 345008 [details]
Patch

[Darin Adler]

Comment on attachment 345008 [details]
Patch

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

Concept here is fine but std::numeric_limits<unit64_t>::max() is probably better than writing -1.

Also if we really want to optimize the size of this it seems strange we really need 64 bits for something like header size. There must be much smaller practical limits for the sizes of various things here. And we could store headers in a sorted vector instead of a map and save a *lot* more space.

I am also concerned that this leaves nothing behind to remind us to keep this small. I could easily imagine someone showing up soon with a “cleanup” patch to make this use std::optional and we are not even leaving a comment behind to prevent that.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:246
> +    if (networkLoadMetrics.requestHeaderBytesSent != -1ull)

-1ull does not make sense. -1 is not an unsigned number. Can we find a correct way to write this?

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 345008 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345008&action=review
> 
> Concept here is fine but std::numeric_limits<unit64_t>::max() is probably
> better than writing -1.
> 
> Also if we really want to optimize the size of this it seems strange we
> really need 64 bits for something like header size. There must be much
> smaller practical limits for the sizes of various things here. And we could
> store headers in a sorted vector instead of a map and save a *lot* more
> space.

HTTPHeaderMap is based on Vector nowadays already, not HashMap.

> 
> I am also concerned that this leaves nothing behind to remind us to keep
> this small. I could easily imagine someone showing up soon with a “cleanup”
> patch to make this use std::optional and we are not even leaving a comment
> behind to prevent that.
> 
> > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:246
> > +    if (networkLoadMetrics.requestHeaderBytesSent != -1ull)
> 
> -1ull does not make sense. -1 is not an unsigned number. Can we find a
> correct way to write this?

[Alex Christensen]

Created attachment 345105 [details]
Patch

[Alex Christensen]

Created attachment 345107 [details]
Patch

[Alex Christensen]

Created attachment 345109 [details]
Patch

[Alex Christensen]

Comment on attachment 345109 [details]
Patch

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

> Source/WebCore/platform/network/NetworkLoadMetrics.h:171
> +    uint32_t requestHeaderBytesSent;
> +    uint32_t responseHeaderBytesReceived;

I used uint32_t for the header bytes.

[WebKit Commit Bot]

Comment on attachment 345109 [details]
Patch

Clearing flags on attachment: 345109

Committed r233863: <https://trac.webkit.org/changeset/233863>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42253400>