OK, Base does use bitfields. That one was my main concern. Child is probably not that important and at the moment worth the effort.

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Honza, do you have a rough estimate what kind of overhead this has per content process?

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #3) > Honza, do you have a rough estimate what kind of overhead this has per > content process? At the moment none, but probably small. OTOH, let you that this class is created for every resource/sub-resource we load via http(s) and also sometimes it's created for security checks and then immediately destroyed. Hence, even a small win may help here.

It looks like reordering the fields we can save 16-bytes (296 -> 280). Switching to a bitfield for the bool vars doesn't save anything, presumably due to needing 16-byte alignment.

This doesn't actually change the size of 64-bit clang, but potentially could prevent bloat if more bools are added.

Eric, thanks for this! But I think the most important is HttpChannelChild now. I'll try to do the review today. If I don't make it, please ask Michal, Valentin or Dragana. The most important to review is if the flags can be used concurrently, since setting a packed flag is no longer an atomic operation. We had bugs in the past about it. Other is that two bit fields can't be compared to each other (it's undefined result or always false, not sure now - but we had bugs because of that too)

(In reply to (away till 13.8.) Honza Bambas (:mayhemer) from comment #8) > Eric, thanks for this! But I think the most important is HttpChannelChild > now. > > I'll try to do the review today. If I don't make it, please ask Michal, > Valentin or Dragana. > > The most important to review is if the flags can be used concurrently, since > setting a packed flag is no longer an atomic operation. We really shouldn't be counting on that, if we're worried about concurrency Atomic<bool> is more appropriate (though of course much larger). > We had bugs in the > past about it. Other is that two bit fields can't be compared to each other > (it's undefined result or always false, not sure now - but we had bugs > because of that too) I wasn't aware of this, if you have a pointer to further discussion I'd love to read it. Regardless, the second patch adding the bitfields didn't have any benefit on 64-bit so I'm fine not landing it.

Patches 3-5 get us down to 2032 bytes for HttpChannelChild, which means mozjemalloc will allocate 2KiB for the object rather than 4KiB. Further efforts past this point probably aren't worth it unless we can get down to 1024 bytes.

Comment on attachment 8994369 [details] [diff] [review] Part 3: Reorder HttpChannelChild member variables Review of attachment 8994369 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/HttpChannelChild.h @@ +298,5 @@ > + // Proxy release all members above on main thread. > + void ReleaseMainThreadOnlyReferences(); > + > +private: > + nsCString mCachedCharset; nit: just one space between tokens. @@ +347,5 @@ > + LABELS_HTTP_CHILD_OMT_STATS mOMTResult = LABELS_HTTP_CHILD_OMT_STATS::notRequested; > + > + uint32_t mCacheKey; > + int32_t mCacheFetchCount; > + uint32_t mCacheExpirationTime; nit: let's stop aligning the member names. @@ +369,5 @@ > + > + // If ResumeAt is called before AsyncOpen, we need to send extra data upstream > + bool mSendResumeAt; > + > + bool mKeptAlive; // IPC kept open, but only for security info nit: comment indentation isn't required anymore. Put it above or one space after ;

Comment on attachment 8994371 [details] [diff] [review] Part 5: Reorder HttpBaseChannel member variables Review of attachment 8994371 [details] [diff] [review]: ----------------------------------------------------------------- Just some minor nits regarding the indentation of some variables. ::: netwerk/protocol/http/HttpBaseChannel.h @@ +543,5 @@ > + // Resumable channel specific data > + nsCString mEntityID; > + // The initiator type (for this resource) - how was the resource referenced in > + // the HTML file. > + nsString mInitiatorType; nit: don't align these. @@ +569,5 @@ > RefPtr<nsHttpConnectionInfo> mConnectionInfo; > nsCOMPtr<nsIProxyInfo> mProxyInfo; > nsCOMPtr<nsISupports> mSecurityInfo; > + nsCOMPtr<nsIHttpUpgradeListener> mUpgradeProtocolCallback; > + nsAutoPtr<nsString> mContentDispositionFilename; nit: don't align this @@ +575,3 @@ > > + RefPtr<nsHttpHandler> mHttpHandler; // keep gHttpHandler alive > + nsAutoPtr<nsTArray<nsCString> > mRedirectedCachekeys; nit: don't align. @@ +728,5 @@ > // Defaults to false. Is set to true at the begining of OnStartRequest. > // Used to ensure methods can't be called before OnStartRequest. > bool mAfterOnStartRequestBegun; > > + bool mRequireCORSPreflight; nit: don't indent

This will vary per site, but just loading techcrunch I ended up with 50+ HttpChannelChild instances in my content process. At 2K per object we're looking at savings in 100s of KB.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5846965a61de00e8ebc7582d30fb98582fe806dd Bug 1240547 - Part 1: Reorder HttpChannelParent member variables. r=valentin https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2140d43498b51b11babcdc550f44c299fcf962 Bug 1240547 - Part 2: Pack HttpChannelParent bool fields. r=valentin https://hg.mozilla.org/integration/mozilla-inbound/rev/0f776009b482ae27b2b8bf646c19a2f47acb2b65 Bug 1240547 - Part 3: Reorder HttpChannelChild member variables. r=valentin https://hg.mozilla.org/integration/mozilla-inbound/rev/2930c95c8a43897fa703471f8e6a706e239035c7 Bug 1240547 - Part 4: Use bitfields for HttpChannelChild bool variables. r=valentin https://hg.mozilla.org/integration/mozilla-inbound/rev/e8fd3709089bb5acd5bbef144901140c0595dc0e Bug 1240547 - Part 5: Reorder HttpBaseChannel member variables. r=valentin

Indentation was cleaned up prior to landing.

https://hg.mozilla.org/mozilla-central/rev/5846965a61de https://hg.mozilla.org/mozilla-central/rev/2e2140d43498 https://hg.mozilla.org/mozilla-central/rev/0f776009b482 https://hg.mozilla.org/mozilla-central/rev/2930c95c8a43 https://hg.mozilla.org/mozilla-central/rev/e8fd3709089b