Closed Bug 373693 Opened 14 years ago Closed 14 years ago

Cycle collector makes AddRef about 6x slower


(Core :: XPCOM, defect)

Not set





(Reporter: bzbarsky, Assigned: peterv)



(Keywords: perf)


(1 file, 2 obsolete files)

Typical numbers from a profile of the testcase in bug 320020:

530 hits under nsDocument::AddRef, of those 440 under 

1279 hits under nsGlobalWindow::AddRef, of those 1027 under nsCycleCollector_forget.

In all, 2396 hits are under nsCycleCollector_forget out of 62777 total hits.

Of the 15185 hits under nsScriptSecurityManager::CheckPropertyAccessImpl, 1158 are under Forget().

Under Forget() things look like this:

In Forget() itself: 487 hits.
Under NS_IsMainThread_P: 1053 hits.

Is there anything we can do to make things better here?  3% of total time on DHTML is pretty nontrivial...  Yeah, we should make CheckPropertyAccessImpl do less AddRef-ing, but still...
Flags: blocking1.9?
Yes, you can make the call to NS_IsMainThread_P a DEBUG-only call. It's a sanity check to ensure that you're not interacting with the cycle collector from non-main threads.
Component: XPCOM → Build Config
Component: Build Config → XPCOM
Flags: blocking1.9? → blocking1.9+
Assignee: nobody → peterv
Attached patch v1 (obsolete) — Splinter Review
Attached patch v1.1 (obsolete) — Splinter Review
Note that this still has NS_IsMainThread and nsCycleCollector_isScanSafe in debug builds, but I think we should keep it that way for the time being. I'll file a separate bug for moving all the other DEBUG-only code behind a define.
Attachment #261482 - Attachment is obsolete: true
Attachment #261526 - Flags: superreview?(dbaron)
Attachment #261526 - Flags: review?(graydon)
Attached patch v1.1Splinter Review
Attachment #261526 - Attachment is obsolete: true
Attachment #261538 - Flags: superreview?(dbaron)
Attachment #261538 - Flags: review?(graydon)
Attachment #261526 - Flags: superreview?(dbaron)
Attachment #261526 - Flags: review?(graydon)
Attachment #261538 - Flags: review?(graydon) → review+
Blocks: 377787
Comment on attachment 261538 [details] [diff] [review]

>-#define NS_SET_PURPLE_BIT(x) ((x) |= (NS_PURPLE_BIT))
>+#define NS_SET_PURPLE_BIT(x) ((x) | (NS_PURPLE_BIT))

I think it would be cleaner to just make the caller do mValue = tmp | NS_PURPLE_BIT than to make this change -- it's very odd for SET and CLEAR to be so different.

Attachment #261538 - Flags: superreview?(dbaron) → superreview+
Boris, any change you could redo a profile for that testcase? This should have made things better.
Yeah.  With the patch, we have 30 hits under nsCycleCollector_forget out of the 90 under nsDocument::AddRef and 123 hits under nsCycleCollector_forget out of the 264 under nsGlobalWindow::AddRef.

Overall, of the 45848 hits in the profile (I didn't run it for quite as long as last time, I guess), 538 are under nsCycleCollector_forget.  That's about 3x less than it used to be.

Under nsCycleCollector_forget, the breakdown is:

269 hits in nsCycleCollector_forget itself.
187 hits in nsCycleCollector::Forget.
The rest in hashtable operations.

Similar improvements for Release()  (bug 373694).
Blocks: 373694
So can this be resolved fixed?  And bug 373694 too?
I was wondering about the same thing, are you satisfied with the fix? ;-) I guess we can always file separate bugs if we think of some other optimisation.
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Oh, I didn't realize you were waiting on me.  Yeah, this patch pretty much made me happy.  30% of AddRef is a lot better than 80%.  ;)
You need to log in before you can comment on or make changes to this bug.