Setting window.status is ridiculously slow

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: bz, Assigned: neil@parkwaycc.co.uk)

Tracking

({fixed-aviary1.0, fixed1.7.5, perf})

Trunk
x86
All
fixed-aviary1.0, fixed1.7.5, perf
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.5 +
blocking-aviary1.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

I was just profiling a DHTML testcase that was nice and laggy and choppy. 
Imagine my surprise when I saw that setting window.status took as much time as
painting and reflow combined!  The total non-idle time was about evenly split
four ways between window.status setting, paintin+reflow, style reresolution, and
various overhead (JS execution, security manager, etc, etc, though see more on
this below).

The relevant part of the profile is:

36346 nsContentTreeOwner::SetStatus(unsigned, unsigned short const*)
  17963 nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&)
  13401 GlobalWindowImpl::GetObjectProperty(unsigned short const*,
        nsISupports**)
   4145 PrepareAndDispatch
    383 nsDocument::FlushPendingNotifications(int, int)
    220 __restore_rt
    205 nsCOMPtr_base::~nsCOMPtr_base()
      3 GlobalWindowImpl::GetDocument(nsIDOMDocument**)
      2 nsXULWindow::GetWindowDOMWindow(nsIDOMWindowInternal**)
      1 nsAString::~nsAString()

Summary:  almost all the time spent on window.status comes from three places:
calling QueryInterface on something, calling GetObjectProperty, and calling
PrepareAndDispatch.

The QueryInterface in question is nsXPCWrappedJS::QueryInterface(), which spends
most of its time in nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject and
XPCConvert::JSObject2NativeInterface.  This is the code in
nsContentTreeOwner::SetStatus that does:

nsCOMPtr<nsIXULBrowserWindow> xulBrowserWindow(do_QueryInterface(xpConnectObj));

(the thing being QIed is an nsBrowserStatusHandler, which was set as the
"XULBrowserWindow" property of the "window" JS object in navigator.js).

The GetObjectProperty part is getting this property off the window, of course.

And PrepareAndDispatch would be the call out to the status handler JS component.

So is there a reason we have all this complexity?  If nothing else, why not just
have a method on nsPIDOMWindow that gets this nsIXULBrowserWindow pointer for
us?  It can cache the pointer and all that, unless we want to support dynamic
changes to the "XULBrowserWindow" thing or something (which I don't think we do).

Maybe we should be focusing more on making the GetObjectProperty and
QueryInterface calls here faster, though?  I'll try to attach the profile to the
bug so people can look at it if desired.
Created attachment 147287 [details]
Profile (gzipped HTML)
Attachment #147287 - Attachment description: Profile -- not a real patch, but bugzilla won't let me attach it otherwise... it's a gzipped HTML file → Profile (gzipped HTML)
Attachment #147287 - Attachment is patch: false
Attachment #147287 - Attachment mime type: text/plain → application/gzip
Is there something we can do to reduce the effective call rate?  Setting the
status bar shouldn't be effective at > ~10Hz.  If we cache the right pointer and
avoid the overhead bz found, would we still want to damp the update frequency?

/be
(Assignee)

Comment 3

14 years ago
Wouldn't it be easier to add an attribute nsIXULBrowserWindow XULBrowserWindow
to one of the interfaces that nsGlobalChromeWindow implements?
Neil, you mean like to nsIDOMChromeWindow?  Yeah, that would work just fine too,
imo.

brendan, that would have to be done very carefully -- we have past bugs about
changes to window.status being "dropped" (and those are probably the reason why
we synchronously flush a reflow when setting the status...).

If no one objects to Neil's suggestion, we should do it and then I'll reprofile.
(Assignee)

Comment 5

14 years ago
It occurs to me that the tabbed browser would like a way to intercept status
changes on its tabs, but that would probably require rearchitecting status :-/
It'd involve passing along the window that it got set on initially, right?
(Assignee)

Comment 7

14 years ago
Come to think of it, would it be possible for the status notifications to crawl
up the dochell tree looking for an nsIWebProgressListener and try to
QueryInterface that to the desired nsIXULBrowserWindow?
Possible, probably.  See GlobalWindowImpl::GetWebBrowserChrome for how we find
the window to dispatch to right now (and see SetStatus and SetDefaultStatus in
that file).

If someone puts up a patch, I can profile it to see how it looks.

Created attachment 147324 [details]
Testcase 2

Short story:  We're about 20 times slower than NS4 on testcase 1.  About the
same as Opera, though.	Konq doesn't show all the numbers.  On testcase 2,
we're about 100-200 times slower than all three of NS4/Opera/Konq, but they
don't update the status bar while we do.

I know for a fact that I've seen "DHTML" tests testing window.status on which
we came out about 5 times slower than Opera in my testing...
(Assignee)

Comment 11

14 years ago
(In reply to comment #0)
>If nothing else, why not just have a method on nsPIDOMWindow that gets this
>nsIXULBrowserWindow pointer for us?
Because nsIXULBrowserWindow is in xpfe and nsPIDOMWindow is in dom?
(In reply to comment #11)
> (In reply to comment #0)
> >If nothing else, why not just have a method on nsPIDOMWindow that gets this
> >nsIXULBrowserWindow pointer for us?
> Because nsIXULBrowserWindow is in xpfe and nsPIDOMWindow is in dom?

Then how about a scriptable "browserContainer" nsISupports attribute (or
whatever we want to call it) in nsIDOMChromeWindow. If we'd do that we'd be one
step closer to getting rid of the silly nsPIDOMWindow::GetObjectProperty()
method. I'd me such a happy camper! :-)
> Because nsIXULBrowserWindow is in xpfe and nsPIDOMWindow is in dom?

Bad reason.  Why not rename nsIXULBrowserWindow to something saner if needed and
move it into DOM?  That is, move the IDL into DOM and leave the impl in XPFE. 
Sorta like we have history apis in docshell and the impl is provided by the
Gecko consumer. This is an equivalent situation.
(Assignee)

Comment 14

14 years ago
Created attachment 147454 [details] [diff] [review]
Lame hack

These are the minimum changes needed to actively cache the status listener.
With that patch, the times on testcase 1 improve a bit (by about 1/10 or so) and
the times on testcase 2 drop 35% or so.
(Assignee)

Comment 16

14 years ago
Comment on attachment 147454 [details] [diff] [review]
Lame hack

>+#include "nsIWebProgress.idl"
>+
>+[scriptable, uuid(1418026e-8f05-4949-84f4-91e4e2956c73)]
>+interface nsIDOMBrowserWindow : nsIWebProgress
Whoops, these should say nsIWebProgressListener :-[
(Assignee)

Comment 17

14 years ago
Comment on attachment 147454 [details] [diff] [review]
Lame hack

>+[scriptable, uuid(1418026e-8f05-4949-84f4-91e4e2956c73)]
>+interface nsIDOMBrowserWindow : nsIWebProgress
>+{
>+  void onLinkIconAvailable(in wstring href);
>+  void init();
>+  void destroy();
>+  void setJSStatus(in wstring status);
>+  void setJSDefaultStatus(in wstring status);
>+  void setOverLink(in wstring link);
>+};
Note that this supercedes nsIXULBrowserWindow...
Neil, are you going to actually have time to work on this?  A lot of the "DOM
performance" tests out there set window.status as part of the test....

Comment 19

14 years ago
One example of benchmark test, which is using window.status for progress in
first test:
  http://www.24fun.com/downloadcenter/benchjs/benchjs.html 
and bad press review based on this test (in Czech language):
  http://www.zive.cz/h/Uzivatel/AR.asp?ARI=117634

MSIE 6.0 SP1 is 9 times faster than Mozilla 1.7 on W2K.
OS: Linux → All
> One example of benchmark test, which is using window.status for progress in
> first test:

Yes, we're well aware.  That was one of the sites I profiled before filing this bug.
(Assignee)

Comment 21

14 years ago
Created attachment 154791 [details] [diff] [review]
Alternate hack

To avoid polluting the chrome window with appshell stuff...
(Assignee)

Comment 22

14 years ago
Created attachment 154817 [details] [diff] [review]
Alternate hack

Diffed correctly this time, doh :-[
Attachment #154791 - Attachment is obsolete: true
Looks reasonable to me, I'd love to sr any diff that removes usage of
GetObjectProperty()! :-)

Comment 24

14 years ago
Can this go on the trunk sometime soon?
(Assignee)

Comment 25

14 years ago
Created attachment 156511 [details] [diff] [review]
Proposed patch
Assignee: general → neil.parkwaycc.co.uk
Attachment #147454 - Attachment is obsolete: true
Attachment #154817 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Attachment #156511 - Flags: superreview?(jst)
Attachment #156511 - Flags: review?(jag)
(Assignee)

Comment 26

14 years ago
Comment on attachment 156511 [details] [diff] [review]
Proposed patch

needs some moa too.
Attachment #156511 - Flags: superreview?(mscott)
Attachment #156511 - Flags: review?(mconnor)

Comment 27

14 years ago
Comment on attachment 156511 [details] [diff] [review]
Proposed patch

sr=mscott for the mozilla\mail changes.
Attachment #156511 - Flags: superreview?(mscott) → superreview+

Comment 28

14 years ago
Comment on attachment 156511 [details] [diff] [review]
Proposed patch

One day we should wrap getting the nsIXULWindow from window in a helper
function.

r=jag on this change.

And yes, to better support tabs we should overhaul the way these status
messages are sent, but perhaps not now.
Attachment #156511 - Flags: review?(jag) → review+
Comment on attachment 156511 [details] [diff] [review]
Proposed patch

sr=jst!
Attachment #156511 - Flags: superreview?(jst)
Comment on attachment 156511 [details] [diff] [review]
Proposed patch

Going for release approval?

/be
Attachment #156511 - Flags: approval1.7.3?
Attachment #156511 - Flags: approval-aviary?

Updated

14 years ago
Flags: blocking1.7.3+
Flags: blocking-aviary1.0+

Comment 31

14 years ago
Comment on attachment 156511 [details] [diff] [review]
Proposed patch

a=asa for branch checkins.
Attachment #156511 - Flags: review?(mconnor)
Attachment #156511 - Flags: approval1.7.3?
Attachment #156511 - Flags: approval1.7.3+
Attachment #156511 - Flags: approval-aviary?
Attachment #156511 - Flags: approval-aviary+
(Assignee)

Comment 32

14 years ago
Created attachment 156692 [details] [diff] [review]
Bustage fix

nsWindowUtils.cpp was including nsXULWindow.h which introduced a bad dependency
on nsXULBrowserWindow.h when all it needed was nsIDocShellTreeItem.h
(Assignee)

Comment 33

14 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed1.7.3
Resolution: --- → FIXED

Comment 34

14 years ago
This seems to have increased Tdhtml by about 10ms:

http://axolotl.mozilla.org/graph/query.cgi?testname=dhtml&tbox=luna.mozilla.org&autoscale=1&days=7&avg=1&showpoint=2004:08:21:07:57:29,1712

Since it decreased Tp, I don't think it's a big deal, but do we know why? I got
the impression this would decrease Tdhtml as well.
It looks like the following DHTML tests slowed down the most:  diagball and
imageslide.  Given that neither sets window.status, I'm not sure what to make of
that.

Note that none of the DHTML tests currently test window.status speed; feel free
to file a bug on me to add such a test?
(Assignee)

Comment 36

14 years ago
An unscientific test suggests that JSBench test 1 is now 20% faster.
Has this been checked in to the aviary branch? if so please add the
fixed-aviary1.0 keyword.
Whiteboard: needed-aviary1.0?

Comment 38

14 years ago
having had a quick look at bonsai and lxr, it doesn't look like this did get
checked in to the aviary branch
Whiteboard: needed-aviary1.0? → needed-aviary1.0

Comment 39

14 years ago
It looks like this did make it into the 1.7 branch so we need it to land on
Aviary too. Who can land this?
Created attachment 161051 [details] [diff] [review]
patch merged to current aviary branch

this is mainly for reference
checked in on aviary
Keywords: fixed-aviary1.0

Updated

14 years ago
Whiteboard: needed-aviary1.0
You need to log in before you can comment on or make changes to this bug.