Closed
Bug 408792
Opened 17 years ago
Closed 17 years ago
Eliminate more QI's from performance hotspots.
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9beta3
People
(Reporter: jst, Assigned: jst)
References
Details
(Keywords: perf)
Attachments
(1 file)
4.83 KB,
patch
|
sicking
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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 | ||
Updated•17 years ago
|
Assignee: nobody → jst
Comment 1•17 years ago
|
||
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+
Comment 3•17 years ago
|
||
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.
Comment 4•17 years ago
|
||
> + 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).
Assignee | ||
Comment 5•17 years ago
|
||
(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 6•17 years ago
|
||
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+
Assignee | ||
Comment 7•17 years ago
|
||
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
Updated•17 years ago
|
Flags: in-testsuite-
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•