Last Comment Bug 655647 - Allow per-process GC/CC from about:memory
: Allow per-process GC/CC from about:memory
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Jiten [:deLta30]
:
: Nicholas Nethercote [:njn]
Mentors:
Depends on: 654041
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-08 20:27 PDT by Nicholas Nethercote [:njn]
Modified: 2011-09-14 07:02 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Added messaging mechanism to send messages to content processes (6.33 KB, patch)
2011-09-08 09:23 PDT, Jiten [:deLta30]
doug.turner: review-
Details | Diff | Splinter Review
Made changes according to suggestions (6.87 KB, patch)
2011-09-11 02:50 PDT, Jiten [:deLta30]
no flags Details | Diff | Splinter Review
Sorry,slight changes were left.Review this one,not the last one. (6.89 KB, patch)
2011-09-11 05:32 PDT, Jiten [:deLta30]
no flags Details | Diff | Splinter Review
Fixed whitespace (6.89 KB, patch)
2011-09-11 16:59 PDT, Jiten [:deLta30]
doug.turner: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-05-08 20:27:35 PDT
Bug 654041 added GC and CC buttons to about:memory that work for the main process.  This is currently fine on desktop but doesn't allow the content process to be GC'd on Fennec.  We should change this by adding GC and CC buttons for every process in about:memory.

Bug 633305 serves as a good guide for this bug.
Comment 1 Jiten [:deLta30] 2011-08-30 10:59:52 PDT
I will be attempting to solve this bug.This would be a good start for me.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-30 11:02:17 PDT
Awesome.  You might want to join #developers or #content on our IRC server to talk to people who know about the multiprocess setup.
Comment 3 Jiten [:deLta30] 2011-08-31 19:04:00 PDT
Proposed approach:
1) Implementing sendAsyncMessage() method in http://mxr.mozilla.org/mozilla-                              central/source/toolkit/components/aboutmemory/content/aboutMemory.js and addMessageListener() method in http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/content.js.
Comment 4 Jiten [:deLta30] 2011-08-31 19:26:35 PDT
2) When "GC" or "CC" buttons are clicked on memory page,a message is sent to http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/content.js and it is received by addMessageListener() method which will trigger methods with same functionality as defined in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/aboutmemory/content/aboutMemory.js#95 and
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/aboutmemory/content/aboutMemory.js#101.

Suggestions?
Comment 5 Nicholas Nethercote [:njn] 2011-08-31 20:23:09 PDT
dougt, IIRC you implemented the multi-process support in about:memory -- how does comment 3 and comment 4 sound to you?
Comment 6 Doug Turner (:dougt) 2011-08-31 21:52:53 PDT
That's pretty specific to about:memory.  Do you think there is ever going to be a need to cause a GC/CC from any where else?  If so, maybe we should hang a "trigger GC/CC" method on PBrowser?
Comment 7 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-09-06 11:25:37 PDT
I think adding an IPDL message is overkill at this point. In general, we try to avoid triggering GC or CC explicitly, and I'm having difficulties imagining when this message would be used outside of this instance. I think going the route of loading a content script and sending messages is the way to go here.
Comment 8 Jiten [:deLta30] 2011-09-06 12:05:13 PDT
Why isn't adding an IPDL message a good idea?
Comment 9 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-09-06 12:35:45 PDT
Because it makes it more likely that other consumers will try to use it as well. We don't want people to be triggering memory management tasks whenever they want.
Comment 10 Doug Turner (:dougt) 2011-09-06 13:28:50 PDT
jdm is probably right, we can always promote a new message.

also, the way that this will be built is that chrome code will probably be able to send a message (via the observer service) that the child will see and instruct that child to gc/cc.  at least that is how the ipc memory report code works now.
Comment 11 Jiten [:deLta30] 2011-09-08 09:23:33 PDT
Created attachment 559198 [details] [diff] [review]
Added messaging mechanism to send messages to content processes

review?
Comment 12 Jiten [:deLta30] 2011-09-08 09:26:44 PDT
Need test cases.
Comment 13 Nicholas Nethercote [:njn] 2011-09-08 18:19:53 PDT
Comment on attachment 559198 [details] [diff] [review]
Added messaging mechanism to send messages to content processes

Jiten, if you want to request review on a patch, click on its "details" link, then select '?' in the "review" drop-down box, then enter the name of a person to do the review.  In this case, it sounds like :dougt is a good person to ask for review.
Comment 14 Doug Turner (:dougt) 2011-09-10 20:25:18 PDT
Comment on attachment 559198 [details] [diff] [review]
Added messaging mechanism to send messages to content processes

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

Looks good.  just need to rename the observer topics.   Lets see another patch, getting close.

::: dom/ipc/ContentParent.cpp
@@ +199,5 @@
>          obs->AddObserver(this, "xpcom-shutdown", PR_FALSE);
>          obs->AddObserver(this, NS_IPC_IOSERVICE_SET_OFFLINE_TOPIC, PR_FALSE);
>          obs->AddObserver(this, "child-memory-reporter-request", PR_FALSE);
>          obs->AddObserver(this, "memory-pressure", PR_FALSE);
> +	obs->AddObserver(this, "doGC", PR_FALSE);

child-gc-request

@@ +200,5 @@
>          obs->AddObserver(this, NS_IPC_IOSERVICE_SET_OFFLINE_TOPIC, PR_FALSE);
>          obs->AddObserver(this, "child-memory-reporter-request", PR_FALSE);
>          obs->AddObserver(this, "memory-pressure", PR_FALSE);
> +	obs->AddObserver(this, "doGC", PR_FALSE);
> +	obs->AddObserver(this, "doCC", PR_FALSE);

child-cc-request

@@ +291,5 @@
>          obs->RemoveObserver(static_cast<nsIObserver*>(this), "memory-pressure");
>          obs->RemoveObserver(static_cast<nsIObserver*>(this), "child-memory-reporter-request");
>          obs->RemoveObserver(static_cast<nsIObserver*>(this), NS_IPC_IOSERVICE_SET_OFFLINE_TOPIC);
> +	obs->RemoveObserver(static_cast<nsIObserver*>(this), "doGC");
> +	obs->RemoveObserver(static_cast<nsIObserver*>(this), "doCC");

same - rename these too.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +96,5 @@
>  {
>    Cu.forceGC();
> +  var os = Cc["@mozilla.org/observer-service;1"]
> +             .getService(Ci.nsIObserverService);
> +    os.notifyObservers(null, "doGC", null);

white space is off here.

@@ +107,5 @@
>          .getInterface(Ci.nsIDOMWindowUtils)
>          .cycleCollect();
> +  var os = Cc["@mozilla.org/observer-service;1"]
> +             .getService(Ci.nsIObserverService);
> +    os.notifyObservers(null, "doCC", null);

same ws problem
Comment 15 Jiten [:deLta30] 2011-09-11 02:50:41 PDT
Created attachment 559752 [details] [diff] [review]
Made changes according to suggestions
Comment 16 Jiten [:deLta30] 2011-09-11 05:32:07 PDT
Created attachment 559759 [details] [diff] [review]
Sorry,slight changes were left.Review this one,not the last one.
Comment 17 Doug Turner (:dougt) 2011-09-11 14:33:25 PDT
Comment on attachment 559759 [details] [diff] [review]
Sorry,slight changes were left.Review this one,not the last one.

This looks okay, but whitespace is off in ContentChild, and in aboutMemory.js
Comment 18 Jiten [:deLta30] 2011-09-11 16:59:57 PDT
Created attachment 559800 [details] [diff] [review]
Fixed whitespace

I have set whitespace at the starting of the line, according to the code above it.
Comment 19 Matt Brubeck (:mbrubeck) 2011-09-14 07:00:57 PDT
https://hg.mozilla.org/mozilla-central/rev/e3cfe5ae818c
Comment 20 Matt Brubeck (:mbrubeck) 2011-09-14 07:02:05 PDT
Oops, wrong URL.  I mean https://hg.mozilla.org/mozilla-central/rev/a1268291394f

Note You need to log in before you can comment on or make changes to this bug.