Closed Bug 408792 Opened 17 years ago Closed 17 years ago

Eliminate more QI's from performance hotspots.

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: jst, Assigned: jst)

References

Details

(Keywords: perf)

Attachments

(1 file)

When running JS that ends up doing a lot of flushes on the document w/o anything changed in the document (which happens any time a property is resolved on the document for instance), a good chunk of the time spent in the flushing code is QI'ing mScriptGlobalObject to nsPIDOMWindow (in nsDocument::GetWindow()). Another place where we spend time QI'ing for no reason is in a couple spots in the document scriptable helper resolve hooks. See attachment for a fix that eliminates this unnecessary overhead.
Flags: blocking1.9+
Attachment #293639 - Flags: superreview?(bzbarsky)
Attachment #293639 - Flags: review?(jonas)
Assignee: nobody → jst
Does this change the behavior when a document is in bfcache or being destroyed?
IIRC, GetScriptGlobalObject() returns null in those cases and so does then
nsDocument::GetWindow().
Though I guess the change to nsDocument::SetScriptGlobalObject
clears also mWindow when necessary, right?
Comment on attachment 293639 [details] [diff] [review]
Eliminate QI's that aren't needed.

Assuming the answer to smaugs question is 'yes', r=me
Attachment #293639 - Flags: review?(jonas) → review+
Could we just use mParentDocument (which I believe is correctly set up nowadays) and skip not only the Window QI but also all the docshell stuff in FlushPendingNotifications?  That would only affect cases when we're flushing layout (since otherwise we don't actually walk up the parent chain), but still...

We might still want to make the GetWindow() change, of course.
> +  nsPIDOMWindow *mWindow; // Weak reference.

Please expand this comment to explain why it's safe to use a weak reference here (and perhaps why we're holding onto the pointer in the first place).
(In reply to comment #1)
> Does this change the behavior when a document is in bfcache or being destroyed?
> IIRC, GetScriptGlobalObject() returns null in those cases and so does then
> nsDocument::GetWindow().
> Though I guess the change to nsDocument::SetScriptGlobalObject
> clears also mWindow when necessary, right?

This doesn't change behavior AFAICT. SetScriptGlobalObject() nulls out mWindow when the script global is cleared, and after that all calls to GetWindow() will fall through to the old code that QIs on the result of calling GetScriptGlobalObject(), which will find the script global through the docshell if we're being destroyed etc, just like we did before this patch.

I think we can probably use mParentDocument in FlushPendingNotifications() as well, but I need a bit more time to look into it and create a patch...
Comment on attachment 293639 [details] [diff] [review]
Eliminate QI's that aren't needed.

sr=bzbarsky, with the comments expanded out.

And please file a followup for the mParentDocument change?  I think we'd like that for 1.9.
Attachment #293639 - Flags: superreview?(bzbarsky) → superreview+
Fix checked in, though for some reason I'm not seeing it in bonsai (?).

Filed bug 409390 on using mParentDocument to eliminate more code and make things even faster.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: