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...
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.
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 #261538 - Flags: review?(graydon) → review+
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
Attachment #261538 - Flags: superreview?(dbaron) → superreview+
Boris, any change you could redo a profile for that testcase? This should have made things better.
Status: NEW → ASSIGNED
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).
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.