558624 - e10s Cookies: Optimize implementation

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Jason Duell]

Right now our cookie implementation uses a Sync IPDL call per cookie lookup (right?).  This may be expensive: it'd be worth benchmarking at some point, to see how high priority this bug is.

It'd be nice to eventually have fewer msgs, and not sync ones.  I don't know if that's a matter of keeping a cookie db on the child updated with changes to chrome's, or of shipping the relevant cookies for a request along in OnStartRequest.

Comment 2

[Alon Zakai (:azakai)]

Attachment: Profiling visualization on cnn.com

Profiling in bug 588050, with the patch to bug 589905 (so prefs don't dominate it), shows that cookies add 0.3 seconds of latency, which is 5% of the total.

Attached image is the messages sent from the child. Red is cookies, green is http, black is other. (One message per line, x axis is time, time is also shown with numbers on the y axis for convenience).

Comment 3

[[e-mail]]

Very cool, thanks!

For 5% I don't think this blocks, but *maybe* we want it for final. Opinions?

Comment 4

[Doug Turner (:dougt)]

5% win for all loads rocks.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

I'd take 5%

Comment 6

[Doug Turner (:dougt)]

here is what I understand about what Dan and I spoke about yesterday.  Please review and offer feedback.  Specifically:

bz - OMR will not work in the child process... read more below

jason - do you see any problem will killing off setting cookies in the child?


1) The cookies database will remain in the parent process.  There is no reason to copy it to the child process like we did with preferences.

2) cookies will be set on the http change only from the parent process.

This means that OMR will NOT work from the child.  Looking at the callers, this is acceptable.  Most are tests, others should only be called in the parent process.  For Addon's, we hope that they will not want OMR in the child.

This also means that much of the code in the child http channel can be removed.


3) Two cases where we need access to the cookies from the content child:

a) META HTTP-EQUIV="Set-Cookie" 

Basically this is a way for web content to set a cookie.

For this, we will change the code that handles the meta tag to remote the cookie to the cookie-service in the parent process.  This message could probably be asynchronous, but since it is called so rarely we probably can call it synchronously.

b) document.cookie

Basically this is a way for web content to set and get a cookie.

For this, similar to the meta tag, we can remote it from the child process to the parent.  It isn't clear if the set needs to be synchronous, but the needs to be.


Thoughts?

Comment 7

[Boris Zbarsky [:bzbarsky]]

"omr"?  On-modify-response?

I wonder whether there are people out there benchmarking document.cookie access....

I'm not sure about sets being sync, or gets, but a get needs to see the results of all previous sets for sure.

Comment 8

[[e-mail]]

(In reply to comment #6)
> 3) Two cases where we need access to the cookies from the content child:
> a) META HTTP-EQUIV="Set-Cookie" 
> b) document.cookie

FWIW what you describe is the way they both work now, so no changes needed there.

(In reply to comment #7)
> "omr"?  On-modify-response?

The "http-on-modify-request" topic. That's the big (and only) question here; whether it's OK for OMR in the child to not expose, or accept mutations to, cookies... (doing so in the parent would still work just fine.)

> I wonder whether there are people out there benchmarking document.cookie
> access....

Maybe. In real code, though, there's no need for it to be a hotspot. (For reasonable cases I can think of, anyway!)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  Can extensions actually change category entries in the child?  That is, can they even install an omr listener?

> In real code, though, there's no need for it to be a hotspot.

Do you have any idea what fraction of benchmark hotspots that's true for?  :(

Comment 10

[Doug Turner (:dougt)]

> Can extensions actually change category entries in the child?

You could build an addon that loads a frame script that tries to listen to OMR in the child.  It wouldn't work after this change.

Comment 11

[[e-mail]]

(In reply to comment #9)
> Do you have any idea what fraction of benchmark hotspots that's true for?  :(

Nope. :(

Regardless, I don't think there's anything we can do about it -- the only solutions are a) send a synchronous message from parent to child to push mutations, which is obviously a nonstarter; b) move all cookie access into the child, and break the multiple-child-process model; c) same as b) but without breaking multiple children, by deliberately changing the cookie model to be per-tab/per-window/whatever. (This is a bit dangerous to jump the gun on, since it ties an implementation detail to user-facing behavior.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yeah, ok.  Sounds like we just need to remote document.cookie...

I'm OK with the OMR thing, I think.

Comment 13

[[e-mail]]

It looks like dougt's all over this.

Comment 14

[Doug Turner (:dougt)]

Attachment: patch 1 - clean up warnings

Comment 15

[Doug Turner (:dougt)]

Attachment: patch 2 - add unit test

Comment 16

[Doug Turner (:dougt)]

Attachment: patch 3 - forward request headers to child in OnStartRequest

Comment 17

[Doug Turner (:dougt)]

Attachment: patch 4 - remove cookies from being set in the child

Comment 18

[Doug Turner (:dougt)]

i'll remove those lines with "dft" and re-indent.

Comment 19

[[e-mail]]

Comment on attachment 480182 [details] [diff] [review]
patch 1 - clean up warnings

r=dwitte

Comment 20

[[e-mail]]

Comment on attachment 480183 [details] [diff] [review]
patch 2 - add unit test

r=dwitte

Comment 21

[[e-mail]]

Comment on attachment 480185 [details] [diff] [review]
patch 3 - forward request headers to child in OnStartRequest

>diff --git a/netwerk/protocol/http/HttpChannelChild.cpp b/netwerk/protocol/http/HttpChannelChild.cpp

>@@ -287,16 +294,22 @@ HttpChannelChild::OnStartRequest(const n

>+  mRequestHead.ClearHeaders();
>+  for (PRUint32 i = 0; i < requestHeaders.Length(); i++) {
>+    mRequestHead.Headers().SetHeader(nsHttp::ResolveAtom(requestHeaders[i].mHeader),
>+                                     requestHeaders[i].mValue);
>+  }

A little comment here saying "replace our request headers with what actually got sent in the parent" would be nice.

>diff --git a/netwerk/protocol/http/nsHttpChannel.h b/netwerk/protocol/http/nsHttpChannel.h

> public: /* internal necko use only */ 
>     typedef void (nsHttpChannel:: *nsAsyncCallback)(void);
>     nsHttpResponseHead * GetResponseHead() const { return mResponseHead; }
>+    nsHttpRequestHead * GetRequestHead() { return &mRequestHead; }

Hmm. Both of these belong on HttpBaseChannel. Can you move them there?

r=dwitte.

Comment 22

[[e-mail]]

Comment on attachment 480186 [details] [diff] [review]
patch 4 - remove cookies from being set in the child

>diff --git a/netwerk/protocol/http/HttpBaseChannel.cpp b/netwerk/protocol/http/HttpBaseChannel.cpp

>@@ -1239,35 +1239,47 @@ HttpBaseChannel::GetEntityID(nsACString&

>+  PRBool useCookieService = 

Use a plain old bool here?

>+#ifdef MOZ_IPC
>+    (XRE_GetProcessType() == GeckoProcessType_Default);
>+#else
>+    PR_TRUE;
>+#endif
>+    
>   nsXPIDLCString cookie;
> 
>-  nsICookieService *cs = gHttpHandler->GetCookieService();
>-  if (cs) {
>-    cs->GetCookieStringFromHttp(mURI,
>-                                mDocumentURI ? mDocumentURI : mOriginalURI,
>-                                this, getter_Copies(cookie));
>+  if (useCookieService) {
>+
>+    nsICookieService *cs = gHttpHandler->GetCookieService();

Extra newline.

>+    if (cs) {
>+      cs->GetCookieStringFromHttp(mURI,
>+                                  mDocumentURI ? mDocumentURI : mOriginalURI,

The second param is unused in cookieservice (permanently) -- you can just pass
NULL.

>diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp

>@@ -1011,22 +1011,17 @@ nsHttpChannel::ProcessResponse()

>-    if (!mRemoteChannel) {

Looks like you're killing off the only usages of mRemoteChannel, and its IDL property:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannelInternal.idl#105

Care to mop that up?

Looks good, r=dwitte.

Comment 23

[Doug Turner (:dougt)]

thanks for the reviews.  I will also mop up the unused internal interface.

Comment 24

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/0a7f37d28f7e
http://hg.mozilla.org/mozilla-central/rev/64be51b42da7
http://hg.mozilla.org/mozilla-central/rev/3a794e272196
http://hg.mozilla.org/mozilla-central/rev/25b8cd0b2767
http://hg.mozilla.org/mozilla-central/rev/7a1028b9d42f