Closed
Bug 580096
Opened 14 years ago
Closed 14 years ago
move the cycle collector off the main thread
Categories
(Core :: XPCOM, defect)
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)
(deleted),
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Comment 2•14 years ago
|
||
I run finalization on the main thread, but just traversal off thread causes a million warnings. Working on that.
Assignee | ||
Comment 3•14 years ago
|
||
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)
Assignee | ||
Comment 4•14 years ago
|
||
Comments certainly welcome from anyone :)
Assignee | ||
Updated•14 years ago
|
Attachment #466725 -
Flags: review?(benjamin) → review?(peterv)
Comment 5•14 years ago
|
||
This is part of cutting down our GC pause times, and thus should block 2.0.
blocking2.0: --- → betaN+
Comment 6•14 years ago
|
||
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+
Assignee | ||
Comment 7•14 years ago
|
||
Unbitrotted, updated with comments.
Attachment #466725 -
Attachment is obsolete: true
Assignee | ||
Comment 8•14 years ago
|
||
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)
Updated•14 years ago
|
blocking2.0: betaN+ → beta8+
Assignee | ||
Comment 9•14 years ago
|
||
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 10•14 years ago
|
||
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+
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #10)
> You need to mark mFlagJSObject in XPCWrappedNative::GetJSObject() too, right?
Er. Yes. Wow.
Updated•14 years ago
|
Attachment #488603 -
Flags: review?(jst) → review+
Assignee | ||
Comment 12•14 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Comment 13•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 14•13 years ago
|
||
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.
Comment 15•13 years ago
|
||
I moved the call to the JS GC to the main thread if that helps anything...
You need to log in
before you can comment on or make changes to this bug.
Description
•