558623 - e10s HTTP: combine PHttpChannel constructor with SendAsyncOpen/SendRedirect1Begin

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

We could save an IPDL message per-channel by combining the PHttpChannel construction with AsyncOpen.  See HttpChannelChild::AsyncOpen.

Not a huge win, but low-hanging fruit.

Comment 1

[Josh Matthews [:jdm]]

Yoink.

Comment 2

[Josh Matthews [:jdm]]

This is not quite as simple as it appears at first.  I took a stab at it, then got bit by the whole nsHttp.h/chromium LOG macro mess again.  I don't really feel like trying to sort that out right now, so I'm removing this from my plate.

Comment 3

[Jason Duell]

This should still be possible.  Note that we'll want to combine both AsyncOpen (for child->parent construction) and SendRedirect1Begin (for parent-> child construction during redirects) into one constructor (since IPDL doesn't allow multiple constructors).   We can do this most easily by using an IPDL union that contains two structs, one the args that AsyncOpen needs, and the other than SendRedirect1Begin needs.  See the TestDataStructures example in ipc/ipdl/tests/cxx.

For info on how to deal with the chromium LOG issue, see bug 545995 and the top of nsHttp.h

Comment 4

[Jason Duell]

Attachment: part1: split out HTTP LOG() stuff into separate file

I hit a higher amount of silly C++ macro/typedef/namespace/etc issues in this patch than any other I can remember writing.  This patch fixes one of them:  we'd been keeping our (kludgy) LOG() macro definitions in nsHttp.h, but the tortuous logic I implemented to ensure we don't conflict with the chromium LOG macro blew up once the IPDL parser needed to see nsHttp.h. (Also we don't want anything but files in /netwerk/protocol/http to see the LOG() macro since it's http specific).  So I split out the log stuff into its own file.  

Note I hg cloned nsHttp.h->HttpLog.h so even though it's a new file you may see deletions in it (not sure how smart splinter will be about that).

Comment 5

[Jason Duell]

Attachment: part2: main patch, v1

My favorite part is where I ran into some X11 header doing "#define Status int" (hello global namespace pollution!).  Glad I remembered my old pal "g++ -E -dD"

Comment 6

[Jason Duell]

https://tbpl.mozilla.org/?tree=Try&rev=e3ee6499bdef

Comment 7

[Jason Duell]

hopefully greener:

  https://tbpl.mozilla.org/?tree=Try&rev=3ed3989a5ca7

Comment 8

[Jason Duell]

Attachment: part2: main patch, v2 (one line fix)

Needed to remove a MOZ_OVERRIDE.

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 763933 [details] [diff] [review]
part1: split out HTTP LOG() stuff into separate file

Review of attachment 763933 [details] [diff] [review]:
-----------------------------------------------------------------

I recall hitting this same problem when I last attempted this and giving up in disgust. You are a better man than I.

Comment 10

[Jason Duell]

Last build seemed to be hitting bustage on inbound.  Here's one off of m-c:

https://tbpl.mozilla.org/?tree=Try&rev=aa32905a019c

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 763992 [details] [diff] [review]
part2: main patch, v2 (one line fix)

Review of attachment 763992 [details] [diff] [review]:
-----------------------------------------------------------------

This is a... special patch. It felt like the code was cleaner before, but fewer messages is a win. So it goes.

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +180,5 @@
>    SerializeInputStream(mUploadStream, uploadStream);
>  
> +  FTPChannelOpenArgs openArgs;
> +  // Gross: no access to FTPChannelOpenArgs members, but they each have a
> +  // function with the struct name that returns a ref.  Yuk.

This is the bed we have made; there's no particular need to grouse about it here.

@@ +187,5 @@
> +  openArgs.entityID() = mEntityID;
> +  openArgs.uploadStream() = uploadStream;
> +
> +  gNeckoChild->SendPFTPChannelConstructor(this, tabChild,
> +                IPC::SerializedLoadContext(this), openArgs);

Indent this to match.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1059,5 @@
>    }
>  
> +  HttpChannelOpenArgs openArgs;
> +  // Gross: no access to HttpChannelOpenArgs members, but they each have a
> +  // function with the struct name that returns a ref.

Preach it, brother.

::: netwerk/protocol/http/HttpChannelParent.h
@@ +55,5 @@
> +  // used to connect redirected-to channel in parent with just created
> +  // ChildChannel.  Used during redirects.
> +  virtual bool ConnectChannel(const uint32_t& channelId);
> +
> +  virtual bool DoAsyncOpen(  const URIParams&           uri,

These two don't need to be virtual.

Comment 12

[Jason Duell]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/312d1be1a95a
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f22c958c6187

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/312d1be1a95a
https://hg.mozilla.org/mozilla-central/rev/f22c958c6187