405128 - Remove some nsDeque use from cycle collector

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Peter Van der Beken [:peterv]]

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.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

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.

Comment 2

[Stuart Parmenter]

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...

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Switched to nsAutoTPtrArray with 4000 elements.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 290590 [details] [diff] [review]
v1.1

This needs an updated patch.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v1.3

Updated to trunk and stop shutdown assertion in DEBUG_CC builds by calling ClearGraph after checking whether more shutdown runs would collect more.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 9

[Peter Van der Beken [:peterv]]

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.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 302875 [details] [diff] [review]
v1.3

a=beltzner, please monitor closely after landing :)

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Patch that was checked in

Comment 12

[chris hofmann]

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