e10s Cookies: Optimize implementation

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: jduell, Assigned: dougt)

Tracking

(Blocks: 1 bug)

Other Branch
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(5 attachments)

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.

Updated

8 years ago
Duplicate of this bug: 598393

Updated

8 years ago
Blocks: 588050

Comment 2

8 years ago
Created attachment 478317 [details]
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

8 years ago
Very cool, thanks!

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

Comment 4

8 years ago
5% win for all loads rocks.
tracking-fennec: --- → 2.0+
(Assignee)

Comment 6

8 years ago
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?
"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

8 years ago
(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!)
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?  :(
(Assignee)

Comment 10

8 years ago
> 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

8 years ago
(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.)
Yeah, ok.  Sounds like we just need to remote document.cookie...

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

Comment 13

8 years ago
It looks like dougt's all over this.
Assignee: dwitte → doug.turner
(Assignee)

Comment 14

8 years ago
Created attachment 480182 [details] [diff] [review]
patch 1 - clean up warnings
Attachment #480182 - Flags: review?(dwitte)
(Assignee)

Comment 15

8 years ago
Created attachment 480183 [details] [diff] [review]
patch 2 - add unit test
Attachment #480183 - Flags: review?(dwitte)
(Assignee)

Comment 16

8 years ago
Created attachment 480185 [details] [diff] [review]
patch 3 - forward request headers to child in OnStartRequest
Attachment #480185 - Flags: review?(dwitte)
(Assignee)

Comment 17

8 years ago
Created attachment 480186 [details] [diff] [review]
patch 4 - remove cookies from being set in the child
Attachment #480186 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #480186 - Flags: review? → review?(dwitte)
(Assignee)

Comment 18

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

Comment 19

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

r=dwitte
Attachment #480182 - Flags: review?(dwitte) → review+

Comment 20

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

r=dwitte
Attachment #480183 - Flags: review?(dwitte) → review+

Comment 21

8 years ago
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.
Attachment #480185 - Flags: review?(dwitte) → review+

Comment 22

8 years ago
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.
Attachment #480186 - Flags: review?(dwitte) → review+
tracking-fennec: 2.0+ → 2.0b2+
(Assignee)

Comment 23

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

Updated

8 years ago
Duplicate of this bug: 577917

Updated

3 years ago
Blocks: 557742
You need to log in before you can comment on or make changes to this bug.