Closed Bug 405128 Opened 13 years ago Closed 13 years ago

Remove some nsDeque use from cycle collector

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: peterv, Assigned: peterv)

Details

(Keywords: memory-footprint)

Attachments

(1 file, 5 obsolete files)

There are two places in the cycle collector where we use a nsDeque (mBuf) but where we don't really need to. We use mBuf to store purple nodes before adding them to the graph, but we could just add them to the graph directly. We also use mBuf to store the PtrInfo's for white nodes while we root/unlink/unroot them, using a nsTPtrArray should work just fine for that. This might help a bit for reducing memory fragmentation.
Attached patch v1 (obsolete) — Splinter Review
I also started counting the white nodes during marking and used the count to change the size of the array once. I ended up using nsVoidArray, there's no way to resize an nsTArray to an exact size (it uses a doubling algorithm). Not sure if we should use an auto-array, the number of white nodes I see is usually more than a thousand, and it can go to tens of thousands.
Attachment #289952 - Flags: superreview?(dbaron)
Attachment #289952 - Flags: review?(dbaron)
Keywords: footprint
in CollectWhite() at least, you're probably better off using nsAutoTArray of at least enough elements to catch some number there

in the other cases, doubling is probably better than nsVoidArray's behavior.  -- you can Compact an nsTArray if you're worried about overhead...
Attached patch v1.1 (obsolete) — Splinter Review
Switched to nsAutoTPtrArray with 4000 elements.
Attachment #289952 - Attachment is obsolete: true
Attachment #290590 - Flags: superreview?(dbaron)
Attachment #290590 - Flags: review?(dbaron)
Attachment #289952 - Flags: superreview?(dbaron)
Attachment #289952 - Flags: review?(dbaron)
Comment on attachment 290590 [details] [diff] [review]
v1.1

This needs an updated patch.
Attachment #290590 - Attachment is obsolete: true
Attachment #290590 - Flags: superreview?(dbaron)
Attachment #290590 - Flags: review?(dbaron)
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #297064 - Flags: superreview?(dbaron)
Attachment #297064 - Flags: review?(dbaron)
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #297064 - Attachment is obsolete: true
Attachment #297064 - Flags: superreview?(dbaron)
Attachment #297064 - Flags: review?(dbaron)
Attachment #301166 - Flags: superreview?(dbaron)
Attachment #301166 - Flags: review?(dbaron)
Attached patch v1.3 (obsolete) — Splinter Review
Updated to trunk and stop shutdown assertion in DEBUG_CC builds by calling ClearGraph after checking whether more shutdown runs would collect more.
Attachment #301166 - Attachment is obsolete: true
Attachment #302875 - Flags: superreview?(dbaron)
Attachment #302875 - Flags: review?(dbaron)
Attachment #301166 - Flags: superreview?(dbaron)
Attachment #301166 - Flags: review?(dbaron)
Comment on attachment 302875 [details] [diff] [review]
v1.3

+    GCGraphBuilder &mBuilder;

some compilers are going to have problems assigning to reference members
with struct assignment; see bug 413447 comment 29.  You're should either
use a constructor with member initializers or just use a pointer instead
of a reference.  (Constructors are generally nice, I think.  You could
even do both if you want.)

-    if (builder.Count()  > 0 || mBuf.GetSize() != 0) {
+    if (builder.Count()  > 0) {

Could you fix the double-space before ">" while you're here?

+    if (pinfo) {
+        cp->UnmarkPurple(root);
+
+        return PR_TRUE;
+    }
+
+    return PR_FALSE;
I'd prefer the unusual case having an early return and the main path of
the function unindented, i.e.:
#   if (!pinfo)
#       return PR_FALSE;
#      
#   cp->UnmarkPurple(root);
#
#   return PR_TRUE;
   
+    mWhiteNodes->Compact();
Why bother?   It's short-lived anyway, and if you use it again, the
allocation might be useful.


Otherwise, looks great.  r+sr=dbaron
Attachment #302875 - Flags: superreview?(dbaron)
Attachment #302875 - Flags: superreview+
Attachment #302875 - Flags: review?(dbaron)
Attachment #302875 - Flags: review+
Comment on attachment 302875 [details] [diff] [review]
v1.3

Stops using a double-ended queue that reallocates a bunch of times, we really just need an array. Allocates the array on the stack and only reallocates once if necessary. Should help a bit with fragmentation.
Attachment #302875 - Flags: approval1.9?
Comment on attachment 302875 [details] [diff] [review]
v1.3

a=beltzner, please monitor closely after landing :)
Attachment #302875 - Flags: approval1.9? → approval1.9+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9beta2 → mozilla1.9beta4
Attachment #302875 - Attachment is obsolete: true
in looking though a bit of early fx3 crash data I ran across a low volume crash that shows us dying near some of the code for this bug.

http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&range_unit=hours&version=Firefox%3A3.0&signature=ageSelectionCallback&range_value=10

here is the stack

0  	xul.dll  	ageSelectionCallback  	 mozilla/xpcom/base/nsCycleCollector.cpp:856
1 	xul.dll 	nsBaseHashtable<nsVoidPtrHashKey, unsigned int, unsigned int>::s_EnumStub 	nsBaseHashtable.h:346
2 	xul.dll 	PL_DHashTableEnumerate 	pldhash.c:724
3 	xul.dll 	nsCycleCollector::BeginCollection 	mozilla/xpcom/base/nsCycleCollector.cpp:2331
4 	xul.dll 	XPCCycleCollectGCCallback 	mozilla/js/src/xpconnect/src/nsXPConnect.cpp:440
5 	js3250.dll 	js_GC 	mozilla/js/src/jsgc.c:3239
6 	js3250.dll 	JS_GC 	mozilla/js/src/jsapi.c:2469
7 	xul.dll 	nsXPConnect::Collect 	mozilla/js/src/xpconnect/src/nsXPConnect.cpp:529
8 	xul.dll 	nsCycleCollector::Collect 	mozilla/xpcom/base/nsCycleCollector.cpp:2277
9 	nspr4.dll 	PR_CallOnceWithArg

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