113528 - Outliner widgets getting invalidated when focus changes

Status: RESOLVED INVALID

(Core :: XUL, defect, P3)

Description

[Scott MacGregor]

Bienvenu and I are noticing a very severe performance regression which started
showing up at the end of last week. If you have a window with an outliner widget
in it and you take focus away from the window and then bring it back, the
outliner widget is getting invalidated causing us to paint all the rows again. 

The view manager had some major changes towards the end of last week (73382).
Could that have caused this?

Comment 1

[Scott MacGregor]

this is now the big hot spot in my quantify data for message display so adding a
link. quantify runs from earlier last week look great. A quantify run from today
shows the extra painting that's going on now. I backed out bienvenu's recent
performance improvement to outliner to reduce the amount of painting outliner
does and that didn't make the problem go away. So it's not caused by that
landing yesterday.

Comment 2

[David :Bienvenu]

To echo what Scott said, this means that we repaint the folder pane and thread
pane (wouldn't surprise me if we're repainting the message pane too). There's no
frame invalidation going on (i.e., the outliner frame isn't getting invalidated,
as near as I can tell - nsLeafFrame::Invalidate isn't getting called in this
situation)

Comment 3

[Scott MacGregor]

The view manager changes could be the cause. According to roc:

"For example, we changed calls to nsIView::SetVisibility,
which does not invalidate the view, to nsIViewManager::SetViewVisibility, which
does invalidate the view."

Comment 4

[David :Bienvenu]

Ok, it's a long and tawdry stack trace that's causing this (and it's worse than
just painting, I think). What happens is that when the window gets focus, the
appshell tries to persist the window size (even if it hasn't changed, as in this
case). This eventually causes a reflow to happen, and the reflow invalidates.
Here's a partial stack trace:

nsBox::RelayoutDirtyChild(nsBox * const 0x05a5f710, nsBoxLayoutState & {...},
nsIBox * 0x05a5f9a4) line 469 + 28 bytes
nsBox::MarkDirty(nsBox * const 0x05a5f9a4, nsBoxLayoutState & {...}) line 288 +
23 bytes
nsBoxFrame::AttributeChanged(nsBoxFrame * const 0x05a5f96c, nsIPresContext *
0x05723028, nsIContent * 0x0437f748, int 0x00000000, nsIAtom * 0x0320b080, int
0x00000001, int 0x00000004) line 1413
nsCSSFrameConstructor::AttributeChanged(nsCSSFrameConstructor * const
0x05722e78, nsIPresContext * 0x05723028, nsIContent * 0x0437f748, int
0x00000000, nsIAtom * 0x0320b080, int 0x00000001, int 0x00000003) line 10296 +
36 bytes
StyleSetImpl::AttributeChanged(StyleSetImpl * const 0x05722d78, nsIPresContext *
0x05723028, nsIContent * 0x0437f748, int 0x00000000, nsIAtom * 0x0320b080, int
0x00000001, int 0xffffffff) line 1470
PresShell::AttributeChanged(PresShell * const 0x04456508, nsIDocument *
0x040b86c0, nsIContent * 0x0437f748, int 0x00000000, nsIAtom * 0x0320b080, int
0x00000001, int 0xffffffff) line 5123 + 61 bytes
nsXULDocument::AttributeChanged(nsXULDocument * const 0x040b86c0, nsIContent *
0x0437f748, int 0x00000000, nsIAtom * 0x0320b080, int 0x00000001, int
0xffffffff) line 2064
nsXULElement::SetAttr(nsXULElement * const 0x0437f748, nsINodeInfo * 0x05910998,
const nsAString & {...}, int 0x00000001) line 2723
nsXULElement::SetAttribute(nsXULElement * const 0x0437f74c, const nsAString &
{...}, const nsAString & {...}) line 1361 + 31 bytes
nsXULWindow::PersistPositionAndSize(nsXULWindow * const 0x057268d0, int
0x00000001, int 0x00000001, int 0x00000001) line 1062 + 59 bytes
nsWebShellWindow::StoreBoundsToXUL(int 0x00000001, int 0x00000001, int
0x00000001) line 1403

It's not immediately obvious what changed recently to cause all these repaints -
we've been persisting the window size on focus for a while, and reflowing in the
view manager on a nsViewManager::SetWindowDimensions call for a while as well.
it seems like the easiest thing to do is see if the window dimensions are really
changing before doing a reflow.

Comment 5

[Scott MacGregor]

cc'ing Eric. On 12/03 he landed some changes to nsBoxFrame::ReflowDirtyChild
among other things which is showing up in David's stack trace. Eric, any chance
your changes could have caused us to start doing extra reflows? Read David's
comments above about why outliner widgets are getting invalidated whenever focus
changes. We are wondering if it could be related to your check in for Bug #110328

Comment 6

[Scott MacGregor]

I've been able to verify that if we comment out the code which persists the
window size when focus changes in nsWebShellWindow::HandleEvent (thank you David
for pointing that out), then we don't run through nsBoxFrame::ReflowDirtyChild,
which marks the outliner as being dirty. Using quantify, I then verified that
the outliner painting numbers return back to last weeks numbers. 

Comment 7

[David :Bienvenu]

I can fix this by changing nsXULWindow::PersistPositionAndSize not to set
attributes that haven't changed. I'll attach a patch that does that. I have no
idea if that's the right thing to do, or if we should catch it at a lower level.
Does the expense of doing a GetAttribute call prohibit making
nsXULElement::SetAttribute check if the attribute has changed? Or could
nsXULElement::SetAttr do it? Or should evaughan look at his change somehow?

Comment 8

[David :Bienvenu]

Attachment: don't change window coords if they haven't changed.

Comment 9

[Scott MacGregor]

dave/evaughan, it'd be great if we can get some traction on this regressin real
soon before we do next weeks round of performance numbers. This has a very
negative impact and hides many of the improvements we've been making. We'd
really appreciate it if you guys can look into this as soon as you can. I feel
like it's been sitting without getting any love from the experts ever since I
filed it back on the 4th....

Comment 10

[[e-mail] (gone - use [e-mail] instead)]

mscott: it's too late for me, unless I'm instructed to test a different build,
it'll be today's build that I test on Tuesday. (always a friday build).

Comment 11

[David Hyatt]

Can I see a full stack so I can see why StoreBoundsToXUL is getting called?

Comment 12

[Scott MacGregor]

Dave I believe it gets called by the NS_GOTFOCUS case in
nsWebShellWindow::HandleEvent

Comment 13

[David Hyatt]

That is just plain wrong.  Focusing a window shouldn't do anything with bounds
storing.

Comment 14

[Scott MacGregor]

hee hee looks like you approved that change last January =). Danm checked that
change in a year ago. Sounds like it may not be what we want to do.

1.324danm%netscape.comJan 3 16:05 save persisted window attributes when window
is activated. bugs 32148,17149. r=hyatt,saari

Comment 15

[Håkan Waara]

This bug is quite ugly. The folderpane is redrawing its rows redundantly almost
every time I click a folder - so it's also very visible.

Can we get some traction? Hyatt, could you give an update on XUL bounds stuff
that you obviously approved last january according to mscott?

(Jan, this is the bug we talked about.)

Comment 16

[Jan Varga [:janv]]

ccing bryner for any ideas

Comment 17

[Jan Varga [:janv]]

Could we just check in the bienvenu's fix at least ?

Comment 18

[Håkan Waara]

Only if people review it first. ;-)   

r=, sr=, anyone?

Comment 19

[Peter Trudelle]

nsbeta1+ per Nav triage team, ->1.0

Comment 20

[Håkan Waara]

Jan? Hyatt? Waterson? Can you have a look at bienvenu's patch? This bug sucks.

Comment 21

[Jan Varga [:janv]]

I'm not right person to review, but the patch makes sense to me in that context
at least.

Comment 22

[Jan Varga [:janv]]

maybe use .AppendInt() instead of PR_snprintf() ?

Comment 23

[Brian Ryner (not reading)]

Comment on attachment 60533 [details] [diff] [review]
don't change window coords if they haven't changed.

r=bryner

As I understand it, from a performance point of view, it's best to not call
SetAttribute if the attribute hasn't changed.

Comment 24

[Jan Varga [:janv]]

I think there is even an assertion (DEBUG_smfr) about calling SetAttr() with the
same value.

Comment 25

[Jan Varga [:janv]]

I think this patch has issues.

1. launch browser and make it bigger
2. launch mailnews
3. close browser
4. launch browser again

result: browser window still has original size
expected result: browser window size is correctly persisted 

Comment 26

[Peter Trudelle]

->Jan

Comment 27

[Jan Varga [:janv]]

Interesting...
Notice that a similar patch was backed out:
1.75 danm%netscape.com Sep 6 2001
remove bug 70283's optimization to persist window size state only if it
(apparently) hasn't changed. bug 86955, 89740. also set a newly opened window's
main widget's zoom state even if it's not yet visible. bug 96475
r=hyatt,pchen

First checked in:
1.70 danm%netscape.com Jun 4 2001
speed up PersistPositionAndSize. bugs 70283, 79060
r=hyatt,saari a=blizzard


I found out that we invalidate outliners (and probably not only outliners)
when we switch focus of elements. e.g. tabbing from message pane to folder pane
element->focus() calls window->focus() and then this calls PersistPositionAndSize()

I think that danm should take a look too.

Comment 28

[Dan M]

Comment on attachment 60533 [details] [diff] [review]
don't change window coords if they haven't changed.

Thanks for pointing this out, Jan! See nsXULWindow.cpp, rev 1.74. It has code
identical to this patch. It was backed out in 1.75 because it didn't work.

Comment 29

[Dan M]

  cc:ing Simon. Hey Simon -- my understanding is that this bug is a performance
problem caused by a reflow when an attribute changes. I remember in some other
(?) bug Simon also considered, basically, the patch in this bug. Instead, he
ended up tweaking the style system somewhere so it would ignore changes to these
attributes. I forget the details. Perhaps that has been backed out?
  Whatever we do, we should add a comment to this code to leave it be. It's an
obvious target for a performance boost, and it's been tried several times.

Comment 30

[Jan Varga [:janv]]

ETA unknown - since I haven't figured out this problem yet.

Comment 31

[Peter Trudelle]

removing plus for reconsideration. What is the current impact of this on target
users, and what is the associated risk of fixing this? It seems time to cut this.

Comment 32

[Jan Varga [:janv]]

Well, here is what I see:
tree gets invalidated when focus changes, but only on trees which have focus ring
e.g: folder pane, thread pane, file picker

When I remove focus ring in mailWindow1.css and then bail out in
FocusRingUpdate_Mail(), trees stop getting invalidated.

Now, I'm investigating how to fix it without removing the focus ring.

Comment 33

[Peter Trudelle]

I'm talking about end user impact; they almost never notice trees being
invalidated.  Is there some noticeable delay, flashing,...?

Comment 34

[Jan Varga [:janv]]

It's no so bad IMHO.
It's only visible on slower machines (additional delay).
I see about 0.5s delay on Linux, 350Mhz machine

Comment 35

[Jan Varga [:janv]]

When I disable focus rings I see no delay, panes switch almost instantly.

Comment 36

[David :Bienvenu]

it's a performance impact, Peter. It causes a completely unnecessary reflow and
repaint, and on slower machines, it's a pain.

Comment 37

[Peter Trudelle]

Thanks, an extra half-second to switch focus is significant. I'm guessing the
risk can't be gauged because there is no fix in hand?

Comment 38

[Jan Varga [:janv]]

one possible cause of this problem - bug 58440

Comment 39

[Jan Varga [:janv]]

ok, so I pulled a tree from 2001-12-01.
I think this bug has been exposed by adding the focus ring to folder pane, since
there is no focus ring in the tree from 2001-12-01.
To be sure, I added the focus ring to folder pane in this tree and then the bug
became visible - outliner/tree widgets with focus ring getting invalidated when 
focus changes.

we have these CSS rules in mailWindow1.css:

#folderTree > .tree-rows > .tree-bodybox,
#threadTree > .tree-rows > .tree-bodybox {
  border: 1px solid transparent;
}

#folderTree:focus > .tree-rows > .tree-bodybox,
#threadTree:focus > .tree-rows > .tree-bodybox {
  border: 1px solid #000000;
}

treechildren is a child of tree-bodybox, so when tree-bodybox changes its style
it invalidates itself and then its children (treechildren).

Comment 40

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

By "invalidates itself and its children", I assume you mean it invalidates its
whole rect.

In theory, for a border-color change, we could just invalidate the four rects
for the border, although it would probably require adding a new style change
hint to the style system, which is a bit of a pain and a good bit of code.  Then
the view manager would do one of two things:
 * merge them, leaving us with the same thing as now
 * paint them separately, causing us to two four passes over the frame tree
instead of one, but much less "real" work on each pass.
I think the view manager does do the latter (recall bug 13131), although I
suspect the optimum would be to choose between them depending on the difference
in area or some other strategy and it may be doing something smarter that might
try to merge the rects.

Which is faster in each case depends on how much is inside the frame, since
walking over the frame tree (in three passes) is not cheap, although it
shouldn't be too expensive for XUL since XUL frames rarely overflow their
parents and thus don't have the NS_FRAME_HAS_OUTSIDE_CHILDREN bit, so we
shouldn't descend into unnecessary frames.  Nevertheless, the XUL frame tree is
also quite deep, so it's not a trivial cost, and I wouldn't be too confident
that invalidating only the border would speed things up.

Comment 41

[Jan Varga [:janv]]

dbaron, thanks for your very useful comments.

I have had an idea to paint focus ring directly in nsTreeBodyFrame
but that would affect scrolling of the tree (see bug 71145).
There is one possible solution to sroll just subrect of the tree body frame,
but nsIWidget:ScrollRect(rect, xDelta, yDelat) is only implemented on windows :(

Comment 42

[Peter Trudelle]

nsbeta1- per Nav triage team, don't see why this is required for MachV, but
let's get it into the point release.

Comment 43

[Peter Trudelle]

renominating for Buffy

Comment 44

[Samir Gehani]

Nav triage team: nsbeta1+/adt2

Comment 45

[Jan Varga [:janv]]

pushing to beta

Comment 46

[Samir Gehani]

Nav triage team: nsbeta1-

Comment 47

[Wayne Mery (:wsmwk)]

(In reply to comment #38)
> one possible cause of this problem - bug 58440

Are Andrew's stacks in bug 370338, which got duped to bug 58440, useful?

Comment 48

[Wayne Mery (:wsmwk)]

Scott thinks "too old to be relevant", so closing INVALID. Please reopen if you think otherwise.