Closed Bug 1240547 Opened 8 years ago Closed 6 years ago

Let HttpChannel* use bit fields for bool flags.

Categories

(Core :: Networking: HTTP, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: mayhemer, Assigned: erahm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-would-take][overhead:100K])

Attachments

(5 files)

      No description provided.
OK, Base does use bitfields.  That one was my main concern.  Child is probably not that important and at the moment worth the effort.
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Whiteboard: [necko-would-take]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
Honza, do you have a rough estimate what kind of overhead this has per content process?
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
Whiteboard: [necko-would-take] → [necko-would-take][overhead:noted]
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.
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Attachment #8993546 - Flags: review?(honzab.moz)
This doesn't actually change the size of 64-bit clang, but potentially could prevent bloat if more bools are added.
Attachment #8993547 - Flags: review?(honzab.moz)
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.
Attachment #8993546 - Flags: review?(honzab.moz) → review?(valentin.gosu)
Attachment #8993547 - Flags: review?(honzab.moz) → review?(valentin.gosu)
Attachment #8994369 - Flags: review?(valentin.gosu)
Attachment #8994370 - Flags: review?(valentin.gosu)
Attachment #8994371 - Flags: review?(valentin.gosu)
Attachment #8993546 - Flags: review?(valentin.gosu) → review+
Attachment #8993547 - Flags: review?(valentin.gosu) → review+
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 ;
Attachment #8994369 - Flags: review?(valentin.gosu) → review+
Attachment #8994370 - Flags: review?(valentin.gosu) → review+
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
Attachment #8994371 - Flags: review?(valentin.gosu) → review+
Whiteboard: [necko-would-take][overhead:noted] → [necko-would-take][overhead:100K]
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.
Indentation was cleaned up prior to landing.
You need to log in before you can comment on or make changes to this bug.