373693 - Cycle collector makes AddRef about 6x slower

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

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

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

Comment 1

[Graydon Hoare :graydon]

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.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1.1

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.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 5

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

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

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

sr=dbaron

Comment 6

[Peter Van der Beken [:peterv]]

Boris, any change you could redo a profile for that testcase? This should have made things better.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

So can this be resolved fixed?  And bug 373694 too?

Comment 9

[Peter Van der Beken [:peterv]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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%.  ;)