38378 - Style system should not reresolve for every attribute change

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Mike Shaver (:shaver emeritus)]

First, some background, as provided to me by the ever-helpful attinasi:

CSSFrameConstructor::AttributeChanged is called. This is were the
restyling happens. First the content object is asked what the impact of
the change is (given an attribute and a value). It can indicate a
RECONSTRUCTION, a REFRAME, or a simple VISUAL change.

For RECONSTRUCT the entire document is torn down and rebuilt. Pretty
harsh.

For a REFRAME, a new frame is created for the affected content, and
normal frame-construction / style resolution takes place.

For a VISUAL change, we ask the FrameManager to compute the style change
impact. This walks up the frames, getting the style contexts from them
and re-resolves the style on each one. ReResolve style runs through all
of the style rules for the content and any pseudo-style associated with
it and sets the new style values. As it goes it checks if the style
context actually changes, and if it does it appends the new context to a
list of changes. It also keeps track of the kinds of changes that are
made, indicating if there needs to be a reflow, a reframe, or just a
repaint. Depending on the results of the resolutions, we then either
reconstruct the frames, issue a reflow, or update (paint) the views.

(End of attinasi missive.)

So, this means that the _best_ case scenario for an attribute change is that we
walk every frame in the document and reresolve the style on each one.  Then we
decide what to do with it, and in the best case again we do nothing (or issue a
paint, I guess).

As an example of why I really want to fix this behaviour, scrolling a GFX
scrollbar sets the ``curpos'' attribute lots.  Like, lots and lots.

So, I hacked up a patch, which I will attach below.  It keeps a list on each
style sheet of each attribute that actually appears in a selector, and then
checks against that table when an attribute changes.  Only if that attribute can
affect style context do we do the re-resolve dance.

With this patch, I spit out a - for each style reresolution that I manage to
skip, and a + for each one that has to happen anyway (no attribute involved in
the reresolution, or attribute that mattered).  I scrolled the checkin-rules
page down and then back up, and got the following:

------------++++++++++--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------++++++----------------------------------------------------------------------------------------------------------++++----------------------------------------------------------------------------++-

So, damn.  It looks like we can win pretty huge here.

But, of course, it's not quite that easy.  I'm apparently causing not _quite_
enough action in the layout engine at times, because some needed updates aren't
happening.  As an example, while the document scrolls, the scroll thumb doesn't
move.  And image rollovers, as found on my old nemesis http://www.honda.com/ ,
don't work.

Maybe I need to fire a paint at the minimum.  Maybe I'm blocking reresolutions
too aggressively, and need to be more careful that I only block the ones that
are directly from the AttributeChanged notification, and not from other paths
through that code.  I'm not sure yet, but I wanted to throw this out for people
to poke at while I ponder.

(I think the patch will build, but some files did get touched lightly when I
updated before the diff, and there are some other -- unrelated, I think --
changes in my layout tree.  No warranty, express or implied.)

Comment 1

[Mike Shaver (:shaver emeritus)]

Attachment: Initial (buggy) patch to reduce reresolution

Comment 2

[Mike Shaver (:shaver emeritus)]

Waterson, as designated Qfy bitch, gets to verify this bug.  Sez me.

Comment 3

[Mike Shaver (:shaver emeritus)]

Progress: scrollbar thumbs move, and rollovers work again (thought honda.com
still looks a little wrong: all the menu items are visible until I roll over one
of them).  Menus still don't, and tooltips neither.

Comment 4

[Mike Shaver (:shaver emeritus)]

Attachment: Improved patch (indicate visual style change for frame in shortcut case)

Comment 5

[Mike Shaver (:shaver emeritus)]

Attachment: Latest patch (including fix for #21879)

Comment 6

[Mike Shaver (:shaver emeritus)]

Hyatt says that the menu badness will go away when he/evaughan lands the new
menu code, because it will no longer rely on style side-effects.  Yay.

My tree now has a better version of the changes, including watching prepended
rules for attribute selectors, and cloning the relevant-attributes table when
we're cloning the sheet.  I'll attach another patch version soonish.

Comment 7

[Mike Shaver (:shaver emeritus)]

Adding evaughan in a plea for him to check in.  There might still be
interactions after the menu/popup stuff is fixed, so I'd like to have at least
24 hours to figure them out before M16 closes.

Apparently, others are also seeing huge gains in scrolling performance,
including threadpane scrolling.  (alecf, if you can quantify threadpane
scrolling gains, it would be nice to see.)

Comment 8

[Steve Morrison]

On my p233/128MB linux box, this 5/7 patch makes scrolling *scream* (i.e., as 
fast as 4.x or any other app). Scrolling with the arrow keys no longer lags. It 
makes mail thread pane scrolling noticeably faster too, it's still a bit 
sluggish. Window resizing, and general browsing seems a bit snappier too, but i 
didn't measure it quantitatively.

Also, some people were wondering if this was Linux only, since this bug is 
marked OS: Linux. I don't think this is the case - is it ok to change to All?

Comment 9

[Mike Shaver (:shaver emeritus)]

All, indeed.

Comment 10

[Mike Shaver (:shaver emeritus)]

Attachment: As above, plus handle Prepended rules and style-sheet cloning.

Comment 11

[Mike Shaver (:shaver emeritus)]

Note to those who would hack on this stuff further:
- we can reduce bloat by using raw PLHashtables rather than the
malloc-unfriendly nsHashtable stuff
- we might need to store the namespace along with the attribute atom, though the
current setup will only result in some missed optimizations, not incorrect
behaviour
- if we store the elements on which the attribute is found, we might be able to
save some additional style reresolutions.  It'd be interesting to instrument to
that end.

Comment 12

[Eric Vaughan]

My menu stuff works on both windows and linux but mac was unhappy. Saari is 
looking at it. Hopefully its simple. So if he gets a fix I'll check in.

Comment 13

[Brendan Eich [:brendan]]

Adding [e-mail] so he can work the dependency.  What's the bug number 
for that dependency, anyway?

/be

Comment 14

[Eric Vaughan]

the dependency is bug #35503

Comment 15

[Mike Shaver (:shaver emeritus)]

Cool.  Can I get the patch, so that I can make sure that my stuff works with it?

Comment 16

[Eric Vaughan]

The patch is rather large it includes many other fixes besides menus. But I 
should be checking in in the next hour. So I'll fire off some mail to you when 
its in and you can pull.

Comment 17

[Mike Shaver (:shaver emeritus)]

A lesser man would be deterred, but I think it's progress.  Now that evaughan

has landed, menus work with my patch, but scrollbars don't appear when they

should.  Scrolling with keys still works, and generates the curpos attribute

settings, though.



What'd you change in scrollbars, Eric?

Comment 18

[Eric Vaughan]

Scrollbars use visiblity collapse to hide or show them. This was needed to make 
them work in scrolling menus. The attribute "collapsed" is being set or unset on 
them. You must make sure you generate a style change or they won't show.

Comment 19

[Mike Shaver (:shaver emeritus)]

I still call AttributeChanged on the scrollbar frame; shouldn't it be queuing a
reflow or whatever if it needs it?

I'd think that we'd want special attribute-reaction stuff to be in the frame's
AttributeChanged handler, so that we don't have to always walk the entire tree
Just In Case.

Comment 20

[Eric Vaughan]

There is a style rule attached to the attribute "collapsed" that maps to 
visibility: collapse. Visibility collapse is inherited. If you change the 
attribute on the scrollbar the stylesystem should cause a stylechange reflow on 
them. Which will cause them to appear or disappear. If they are not showing up 
that is all I can think of. Or you make need to clobber your build. It could be 
a dependency problem.

Comment 21

[Mike Shaver (:shaver emeritus)]

Actually, the problem is that I was failing to check child style sheets for
attribute-relevance.  I think I've got it fixed now.

Comment 22

[Mike Shaver (:shaver emeritus)]

Say my _name_, Gecko.

I think I nailed it.  Can people on other platforms or with more devious minds
give this a quick whirl?  I _really_ want this in M16.

Comment 23

[Mike Shaver (:shaver emeritus)]

Attachment: now look in child sheets, handle media rules and smell nicer

Comment 24

[Mike Shaver (:shaver emeritus)]

You really do want this in for PR2, I think.

Comment 25

[Brendan Eich [:brendan]]

Works for me on win32.

/be

Comment 26

[Mike Shaver (:shaver emeritus)]

Fixed, r=brendan.  smfr promises to wipe my Mac-chin if anything goes awry
there.

Comment 27

[Marc Attinasi]

Shaver, your changes look excellent - great work! 

We need to update the SizeOf method to include the new bloat added to the 
StyleSheetInner (mRelevantAttributes hash table has some overhead, we need to 
measure it) - I doubt it will be much, but we don't want the Sizer code 
bit-rotting. You wanna do it or you want me to?

Comment 28

[Mike Shaver (:shaver emeritus)]

Can you do it?  I'd love you for it.  (Though I should really use a raw
PLHashtable to avoid nsHashtable bloat and allocation badness.)

dmose is reporting that I cause a problem, though, in that Mozilla doesn't
correctly repaint the content area after it's obscured and exposed on Linux,
though the chrome repaints correctly.  It smells like I'm uncovering a previous
bug (or perhaps an optimization to reduce the painting in light of the old
reresolution pain), but I'd really love it if someone could at least track down
which attribute is getting ``incorrectly'' optimized away.

Pav? bliz? can you help?

Comment 29

[Christopher Blizzard (:blizzard)]

The widget not being repainted was not your bug, it was a bug in some of the
widget code that I checked in earlier today.  I've backed myself out until I can
figure out why it's broken.

Comment 30

[Mike Shaver (:shaver emeritus)]

Not done here yet: changing .style doesn't reresolve style correctly.  attanasi,
what style sheet is created for <ELEMENT STYLE="..."> ?

(I fear that the same thing will hit me for CLASS changes.  Maybe
nsGenericHTMLElement::AttributeChanged needs to queue a restyle/reflow if its
class changes?)

Comment 31

[leger]

Putting on [nsbeta2-] radar. Not critical to beta2.  Putting on nsbeta3 list.

Comment 32

[Mike Shaver (:shaver emeritus)]

What are you [nsbeta2-]ing, exactly?  The performance implications of this work
are, well, astounding in the scrolling case, but more importantly the majority
of the code is in the tree.  The only stuff left to fix now are bugs that have
significantly negative effects on things like DHTML.  Those bugs are largely
separately filed, but I'm afraid that you won't take fixes for them for PR2.

I guess it _is_ your beta, but I'd like to know exactly what you mean by
[nsbeta2-].

(Removing [nsbeta2-] for further review.)

Comment 33

[Brendan Eich [:brendan]]

Shaver, what are the other bugs (the DHTML ones) induced by the changes landed
to fix this one?  Maybe they can be linked as dependencies of this bug.  Should
this bug be closed, if new bugs have been opened to describe flaws in the new
code?

Anyway, this optimization is here to stay, so I don't see how this bug can be
nsbeta2- either.

/be

Comment 34

[Marc Attinasi]

Bug 39642 is a related bug for DHTML - HTML attribute changes do not always 
trigger a style re-resolve. 

Shaver, can this bug be closed and the more specific bug 39642 be used to track 
the remaining defect? And any other problems that come up can get their own 
specific bugs as well? BTW, myself and my mgr. karnaze are lobbying to get the 
fix for 39642 approved asap - we cannot go beta2 without fixing it.

Comment 35

[Mike Shaver (:shaver emeritus)]

Attinasi, ever the voice of reason.