Closed
Bug 655647
Opened 14 years ago
Closed 13 years ago
Allow per-process GC/CC from about:memory
Categories
(Toolkit :: about:memory, defect)
Toolkit
about:memory
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: n.nethercote, Assigned: deLta30)
References
Details
(Whiteboard: [MemShrink:P2])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•13 years ago
|
Assignee: nnethercote → nobody
Component: General → about:memory
QA Contact: general → about.memory
Assignee | ||
Comment 1•13 years ago
|
||
I will be attempting to solve this bug.This would be a good start for me.
Awesome. You might want to join #developers or #content on our IRC server to talk to people who know about the multiprocess setup.
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
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?
Reporter | ||
Comment 5•13 years ago
|
||
Reporter | ||
Updated•13 years ago
|
Whiteboard: [MemShrink]
Comment 6•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 8•13 years ago
|
||
Why isn't adding an IPDL message a good idea?
Comment 9•13 years ago
|
||
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•13 years ago
|
||
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.
Reporter | ||
Updated•13 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 11•13 years ago
|
||
review?
Assignee | ||
Comment 12•13 years ago
|
||
Need test cases.
Reporter | ||
Comment 13•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Attachment #559198 -
Flags: review?(doug.turner)
Comment 14•13 years ago
|
||
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
Attachment #559198 -
Flags: review?(doug.turner) → review-
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #559752 -
Flags: review?(doug.turner)
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #559752 -
Attachment is obsolete: true
Attachment #559752 -
Flags: review?(doug.turner)
Attachment #559759 -
Flags: review?(doug.turner)
Updated•13 years ago
|
Attachment #559198 -
Attachment is obsolete: true
Comment 17•13 years ago
|
||
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
Assignee | ||
Comment 18•13 years ago
|
||
I have set whitespace at the starting of the line, according to the code above it.
Attachment #559759 -
Attachment is obsolete: true
Attachment #559759 -
Flags: review?(doug.turner)
Attachment #559800 -
Flags: review?(doug.turner)
Updated•13 years ago
|
Attachment #559800 -
Flags: review?(doug.turner) → review+
Updated•13 years ago
|
Keywords: checkin-needed
Updated•13 years ago
|
Assignee: nobody → jitenmt
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [MemShrink:P2] → [MemShrink:P2][inbound]
Comment 19•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][inbound] → [MemShrink:P2]
Target Milestone: --- → mozilla9
Comment 20•13 years ago
|
||
Oops, wrong URL. I mean https://hg.mozilla.org/mozilla-central/rev/a1268291394f
You need to log in
before you can comment on or make changes to this bug.
Description
•