Summary: | Reduce sizeof(NetworkLoadMetrics) | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | Basuke.Suzuki, cdumez, cgarcia, darin, don.olmstead, ews-watchlist, galpeter, ggaren, hi, Hironori.Fujii, japhet, joepeck, pangle, rniwa, sam, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2021-06-14 12:02:33 PDT
Created attachment 431351 [details]
Patch
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. Created attachment 435928 [details]
Patch
Created attachment 435959 [details]
Patch including soup implementation
It also fixes a couple of redundant move in return statement issues.
Created attachment 436357 [details]
Patch
Created attachment 436366 [details]
Patch
Created attachment 436964 [details]
Patch
After r281860 this needs rebasing which will add another use of emptyMetrics, but please review. The cURL side looks fine from our side. 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. (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. Created attachment 438882 [details]
Patch
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]. |