e10s HTTP: disable cache and any other unused large memory consumers

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: jduell, Assigned: jduell)

Tracking

unspecified
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Now that we're shipping HTTP to chrome we should turn off the disk and memory cache in the child process.  There may be other HTTP data structures we can disable as well.  We need to keep the atom table, and we're keeping the socket transport thread around to service FTP.

Comment 1

9 years ago
We should fix FTP. I thought we decided that we weren't going to support loading FTP content in-browser, just downloading it, and so the content process didn't need to handle FTP. We shouldn't have:

* any socket transport goop
* NSS/PSM
* caches

We should probably add assertions/aborts to the constructors of those services so that we see them.
I agree about NSS/PSM.  We may need to keep a small cache to support FTP directory listings.

For FTP see bug 536289.  Perhaps we're on the wrong track there, but the plan has been to keep it running in content.
Over to Michal to work on at least disabling the disk cache in the child process.
Assignee: nobody → michal.novotny
As mentioned in the conference call today, we're going to turn off the entire socket transport thread and not have FTP run in content, so this may mean you can turn off more things than previously indicated.  Feel free to open separate bugs as seems sensible.
Created attachment 443504 [details] [diff] [review]
turn off the cache
Attachment #443504 - Flags: review?(jduell.mcbugs)
Created attachment 443506 [details] [diff] [review]
turn off socket transport service
Attachment #443506 - Flags: review?(jduell.mcbugs)
The DNS service is used in the child process by nsHTMLDNSPrefetch but AFAICS this doesn't work correctly because the prefetch should be done in the parent process. So we should call DNS service via IPC and disable it in the child, right?
Comment on attachment 443504 [details] [diff] [review]
turn off the cache

Looks fine to me, but I want biesi to look at it if possible, just to be sure.
Attachment #443504 - Flags: superreview?(cbiesinger)
Attachment #443504 - Flags: review?(jduell.mcbugs)
Attachment #443504 - Flags: review+
Comment on attachment 443506 [details] [diff] [review]
turn off socket transport service

Also looks fine, but I want biesi to look at it if possible, just to be sure.
Attachment #443506 - Flags: superreview?(cbiesinger)
Attachment #443506 - Flags: review?(jduell.mcbugs)
Attachment #443506 - Flags: review+
> The DNS service is used in the child process by nsHTMLDNSPrefetch but AFAICS
this doesn't work correctly because the prefetch should be done in the parent
process. So we should call DNS service via IPC and disable it in the child,
right?

That makes sense to me.  Cc'ing les biez, as they ought to know for sure.
Attachment #443504 - Flags: superreview?(cbiesinger) → superreview+
Comment on attachment 443506 [details] [diff] [review]
turn off socket transport service

+++ b/netwerk/protocol/http/src/nsHttpHandler.cpp
+#ifdef MOZ_IPC
+    if (!mozilla::net::IsNeckoChild()) {
+#endif
     rv = InitConnectionMgr();
     if (NS_FAILED(rv)) return rv;
+#ifdef MOZ_IPC
+    }
+#endif

I think this one would be better inside of InitConectionMgr, that way it can better ensure that it doesn't get created. Note that there is another call to InitConnectionMgr in this file.
Attachment #443506 - Flags: superreview?(cbiesinger) → superreview-
Yeah, dns prefetch needs to move out of the child process.
Note that we don't want to land these patches until we've landed FTP in chrome, etc.
Created attachment 449010 [details] [diff] [review]
turn off socket transport service v2
Attachment #443506 - Attachment is obsolete: true
Attachment #449010 - Flags: superreview?(cbiesinger)
Attachment #449010 - Flags: superreview?(cbiesinger) → superreview+
The two patches in this bug are ready to land, it seems. Can someone get them in? Is there more to do here, past those two patches?
Keywords: checkin-needed
I'll create a patch to turn off the DNS service too.

The patches can't land until bugs 536289 and 565163 are fixed.

Updated

8 years ago
tracking-fennec: --- → 2.0b1+
If the patches can't land yet, please don't mark them checkin-needed.
Keywords: checkin-needed
I'm taking this.  Hope to have done very soon.  Some clients may need changes to behave well in complete absence of nsICacheService.
Assignee: michal.novotny → jduell.mcbugs
Bug 536289 is still opened, so these patches can't land.
We should be able to land the patch that disables the cache, which is the big size win, and split disabling the socket transport thread into a different bug.

The existing FTP code handles not having a cache service.  I'm going through the other protocols and uses of the cache service to see if there's anything that needs tweaking to work w/o the cache svc.

Updated

8 years ago
Depends on: 561085
Turning off the cache is possibly problematic for wyciwyg channels (in unmodified state--we have patch for e10s version in bug 561085) 

In interim, landed bug 595293, which turns off disk cache in the child, and shrinks memory cache to 1 MB.

This bug should be changed to blocking-fennec-2.0
tracking-fennec: 2.0b1+ → ?

Comment 23

8 years ago
I'd argue for b2+, since wyciwyg and FTP are marked as such, so we should be able to do this all in one fell swoop. :)
tracking-fennec: ? → 2.0b2+
Comment on attachment 443504 [details] [diff] [review]
turn off the cache

Note that we should remove the changes from the patch for bug 595293 when we land this.
Created attachment 484445 [details] [diff] [review]
Turn off socket transport service in the child process

Unbitrot.

Updated

8 years ago
Attachment #484445 - Attachment is obsolete: true
Created attachment 484446 [details] [diff] [review]
Turn off socket transport service in the child process

Unbitrot.
Created attachment 484448 [details] [diff] [review]
Turn off cache in the child process

Updated

8 years ago
Attachment #449010 - Attachment is obsolete: true

Updated

8 years ago
Attachment #443504 - Attachment is obsolete: true
Turning off the socket transport is a nonstarter, it turns out, since it's needed by nsServerSocket which is created by httpd.js which is launched in child netwerk ipc tests.
Hmm, allegedly child processes can't open sockets in Android (or maybe that's just regular file handles?).  I wonder how httpd.js works in Android if that's true.
Comment on attachment 484448 [details] [diff] [review]
Turn off cache in the child process

Passes try. Let's land and open a followup for turning off the socket transport thread (or not).
Attachment #484448 - Flags: review+
Keywords: checkin-needed
Oh, and please back out the patch from 

  http://hg.mozilla.org/mozilla-central/rev/d20225a9200a

as part of landing this.  They won't break anything if they remain, but they're now superfluous with the cache turned off.
http://hg.mozilla.org/mozilla-central/rev/4ca3a0191455
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Ack, forgot about comment 31.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This bug is resolved.  We can still back out the push from bug 595293, but it's not going to cause any problems if we don't.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Mother always told me to compile my patches on at least one platform before checking them in to mozilla-central :)

Re-pushed backout of bug 595293:

  http://hg.mozilla.org/mozilla-central/rev/b67099fcd4d8
You need to log in before you can comment on or make changes to this bug.