Closed Bug 580096 Opened 9 years ago Closed 9 years ago

move the cycle collector off the main thread

Categories

(Core :: XPCOM, defect)

Other Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: gal, Assigned: bent.mozilla)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 4 obsolete files)

This patch doesn't make the cycle collector concurrent or interruptible. It merely moves it into its own thread. This reduces main thread cache pollution due to cycle collection.
Attached patch patch (obsolete) — Splinter Review
I run finalization on the main thread, but just traversal off thread causes a million warnings. Working on that.
Attached patch Patch, v2 (obsolete) — Splinter Review
This takes Andreas' patch and fixes everything up to run with no warnings and no shutdown hangs. Looks like it passes try server too!
Assignee: gal → bent.mozilla
Attachment #458483 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #466725 - Flags: review?(benjamin)
Comments certainly welcome from anyone :)
Attachment #466725 - Flags: review?(benjamin) → review?(peterv)
This is part of cutting down our GC pause times, and thus should block 2.0.
blocking2.0: --- → betaN+
Comment on attachment 466725 [details] [diff] [review]
Patch, v2

I didn't see any specific issues but please double-check that it compiles with DEBUG_CC set.

Should probably comment somewhere that when we QI to nsCycleCollectionISupports we don't addref.

Given that we don't addref for nsCycleCollectionISupports I'd really like to avoid the NS_ASSERT_OWNINGTHREAD_OR_CCTHREAD changes.

>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp

>@@ -879,21 +879,23 @@ nsGlobalWindow::ShutDown()

>+    nsCOMPtr<nsISupports> kungFuDeathGrip(supports);

Why do you need the kungFuDeathGrip?

>@@ -6058,21 +6060,23 @@ nsGlobalWindow::CacheXBLPrototypeHandler

>+    nsCOMPtr<nsISupports> kungFuDeathGrip(thisSupports);

Why do you need the kungFuDeathGrip?

>diff --git a/js/src/xpconnect/src/nsXPConnect.cpp b/js/src/xpconnect/src/nsXPConnect.cpp

>-nsXPConnect::FinishCycleCollection()
>+nsXPConnect::FinishTraverse()

s/FinishCycleCollection/FinishTraverse/ in the assertion in nsXPConnect::BeginCycleCollection too.

>diff --git a/xpcom/base/nsCycleCollector.cpp b/xpcom/base/nsCycleCollector.cpp

> static nsISupports *
> canonicalize(nsISupports *in)
> {
>-    nsCOMPtr<nsISupports> child;
>+    nsISupports* child;

Hopefully all QI implementations don't fail or set the pointer to null on failure. Should be ok I think.

>     in->QueryInterface(NS_GET_IID(nsCycleCollectionISupports),
>-                       getter_AddRefs(child));
>-    return child.get();
>+                       reinterpret_cast<void**>(&child));
>+    return child;

>+PRBool
>+nsCycleCollector::PrepareForCollection(nsTPtrArray<PtrInfo> *whiteNodes)

>+        return false;

The signature should be bool or this should be PR_FALSE.

>+nsCycleCollector::CleanupAfterCollection()
>+{
> 

Remove this blank line.

>     mWhiteNodes = nsnull;

> nsCycleCollector::FinishCollection()

>+#ifdef COLLECT_TIME_DEBUG
>+    printf("cc: RootWhite() took %lldms\n",
>+           (PR_Now() - now) / PR_USEC_PER_MSEC);
>+#endif
>+
>+
>+#ifdef COLLECT_TIME_DEBUG
>+    now = PR_Now();
>+#endif

These two ifdef'ed blocks can be combined.

>+nsCycleCollector_gccallback(JSContext *cx, JSGCStatus status)
>+{
>+    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>+    NS_WARN_IF_FALSE(sCollectorRunner, "No runner?!");
>+
>+    if (sCollectorRunner)
>+        sCollectorRunner->JSGCHasRun();
>+
>+    nsCOMPtr<nsIJSRuntimeService> rts(do_GetService(nsIXPConnect::GetCID()));
>+    NS_WARN_IF_FALSE(rts, "Failed to get XPConnect?!");
>+    if (rts)
>+        rts->UnregisterGCCallback(nsCycleCollector_gccallback);

Don't we want status to be JSGC_END before calling JSGCHasRun and unregistering?

>diff --git a/xpcom/threads/nsIThreadManager.idl b/xpcom/threads/nsIThreadManager.idl

>+
>+  /**
>+   *
>+   */
>+  readonly attribute boolean isCycleCollectorThread;

Needs non-empty docs.
Attachment #466725 - Flags: review?(peterv) → review+
Attached patch Patch, v2.1 (obsolete) — Splinter Review
Unbitrotted, updated with comments.
Attachment #466725 - Attachment is obsolete: true
Attached patch Patch, v2.2 (obsolete) — Splinter Review
Updated to accommodate bug 609501, and with the changes needed to protect the mFlatJSObject of XPCWrappedNative from being handed out unmarked.
Attachment #488085 - Attachment is obsolete: true
Attachment #488093 - Flags: review?(jst)
blocking2.0: betaN+ → beta8+
Attached patch Patch, v2.3Splinter Review
Ok, this incorporates the changes we discussed for traversing and tracing the rootset things under the map lock. Haven't seen any deadlocks with worker tests. Also fixes the XBL traverse code to not addref, then fixes the isupports macros to warn about addref/release on cc thread.
Attachment #488093 - Attachment is obsolete: true
Attachment #488603 - Flags: review?(jst)
Attachment #488093 - Flags: review?(jst)
Comment on attachment 488093 [details] [diff] [review]
Patch, v2.2

You need to mark mFlagJSObject in XPCWrappedNative::GetJSObject() too, right?

r=jst with that.
Attachment #488093 - Flags: review+
(In reply to comment #10)
> You need to mark mFlagJSObject in XPCWrappedNative::GetJSObject() too, right?

Er. Yes. Wow.
Attachment #488603 - Flags: review?(jst) → review+
Blocks: 613174
http://hg.mozilla.org/mozilla-central/rev/b07a1861acf2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 618365
Depends on: 633843
I don't see any measurements in the comments regarding measured speedup -- were any performed?  The theory makes sense.  I ask b/c supporting this will require hacks in bug 650411 and I'd like to know that those hacks are indeed justified by some measurable improvement.
I moved the call to the JS GC to the main thread if that helps anything...
Blocks: 1139361
You need to log in before you can comment on or make changes to this bug.