Closed Bug 38378 Opened 24 years ago Closed 24 years ago

Style system should not reresolve for every attribute change

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: shaver, Assigned: shaver)

References

Details

(Keywords: perf, Whiteboard: [whitebox])

Attachments

(5 files)

Initial (buggy) patch to reduce reresolution
12.39 KB, patch
Details | Diff | Splinter Review
Improved patch (indicate visual style change for frame in shortcut case)
10.12 KB, patch
Details | Diff | Splinter Review
Latest patch (including fix for #21879)
13.56 KB, patch
Details | Diff | Splinter Review
As above, plus handle Prepended rules and style-sheet cloning.
14.84 KB, patch
Details | Diff | Splinter Review
now look in child sheets, handle media rules and smell nicer
15.90 KB, patch
Details | Diff | Splinter Review 
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.)
Waterson, as designated Qfy bitch, gets to verify this bug.  Sez me.
Status: NEW → ASSIGNED
QA Contact: chrisd → waterson
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.

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.
Keywords: perf
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.)

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?
All, indeed.
Severity: normal → major
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → M16
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.

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.

Adding saari@netscape.com so he can work the dependency.  What's the bug number 
for that dependency, anyway?

/be
the dependency is bug #35503

Cool.  Can I get the patch, so that I can make sure that my stuff works with it?
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.
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?


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.
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.

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.
Actually, the problem is that I was failing to check child style sheets for
attribute-relevance.  I think I've got it fixed now.

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.

You really do want this in for PR2, I think.
Keywords: nsbeta2
Works for me on win32.

/be
Fixed, r=brendan.  smfr promises to wipe my Mac-chin if anything goes awry
there.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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?
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?

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.
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?)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Putting on [nsbeta2-] radar. Not critical to beta2.  Putting on nsbeta3 list.
Keywords: nsbeta3
Whiteboard: [nsbeta2-]
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.)
Whiteboard: [nsbeta2-]
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
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.
Attinasi, ever the voice of reason.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Whiteboard: [whitebox]
No longer blocks: 21762
Blocks: 21762
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: