Bug 226982 – Reduce sizeof(NetworkLoadMetrics)

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

[Alex Christensen]

Use bit fields to reduce sizeof(SecurityOrigin) and sizeof(NetworkLoadMetrics)

[Alex Christensen]

Created attachment 431351 [details]
Patch

[Chris Dumez]

Comment on attachment 431351 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Use bit fields to reduce sizeof(SecurityOrigin) and sizeof(NetworkLoadMetrics)

Why? Do we keep a lot of SecurityOrigins in memory? I did not expect us to.
I don't think we should use bitfields unless there is a strong argument to reduce memory usage, given that it makes the code less readable/convenient.

[Radar WebKit Bug Importer]

<rdar://problem/79573792>

[Alex Christensen]

Created attachment 435928 [details]
Patch

[Carlos Garcia Campos]

Created attachment 435959 [details]
Patch including soup implementation

It also fixes a couple of redundant move in return statement issues.

[Fujii Hironori]

Created attachment 436357 [details]
Patch

[Fujii Hironori]

Created attachment 436366 [details]
Patch

[Alex Christensen]

Created attachment 436964 [details]
Patch

[Alex Christensen]

After r281860 this needs rebasing which will add another use of emptyMetrics, but please review.

[Don Olmstead]

The cURL side looks fine from our side.

[Chris Dumez]

Comment on attachment 436964 [details]
Patch

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

r=me with changes

> Source/WebCore/platform/network/NetworkLoadMetrics.h:67
> +    NetworkLoadMetrics(const NetworkLoadMetrics&) = default;

Why do we need these? What prevents the fault ones from being generated?

> Source/WebCore/platform/network/NetworkLoadMetrics.h:116
> +struct AdditionalNetworkLoadMetricsForWebInspector : public RefCounted<AdditionalNetworkLoadMetricsForWebInspector> {

Given that this subclasses RefCounted, we should make the constructor private and have a public create() function. It is too error-prone otherwise.

Alternatively, looking at your patch, I don't think we need to subclass RefCounted at all as it doesn't seem we ever share ownership of such objects? If so, I would just not subclass RefCounted and use makeUnique / unique_ptr.

[Alex Christensen]

(In reply to Chris Dumez from comment #11)
> Comment on attachment 436964 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436964&action=review
> 
> r=me with changes
> 
> > Source/WebCore/platform/network/NetworkLoadMetrics.h:67
> > +    NetworkLoadMetrics(const NetworkLoadMetrics&) = default;
> 
> Why do we need these? What prevents the fault ones from being generated?
I tried to remove the copy constructor in an earlier version of the patch, and I think this change isn't necessary now.  Will remove.

> > Source/WebCore/platform/network/NetworkLoadMetrics.h:116
> > +struct AdditionalNetworkLoadMetricsForWebInspector : public RefCounted<AdditionalNetworkLoadMetricsForWebInspector> {
> 
> Given that this subclasses RefCounted, we should make the constructor
> private and have a public create() function. It is too error-prone otherwise.
> 
> Alternatively, looking at your patch, I don't think we need to subclass
> RefCounted at all as it doesn't seem we ever share ownership of such
> objects? If so, I would just not subclass RefCounted and use makeUnique /
> unique_ptr.
The copy constructor copies a reference.  It would be an unnecessary copy if it were a unique_ptr.

[Alex Christensen]

Created attachment 438882 [details]
Patch

[EWS]

Committed r282853 (241984@main): <https://commits.webkit.org/241984@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438882 [details].