39618 - Style Contexts are taking up way too much memory

Status: VERIFIED FIXED

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

Description

[Marc Attinasi]

Style Contexts are large and there are often hundreds or thousands of them for a 
large page. They account for 1MB of memory on cnn.com, for example.

A better strategy to share style contexts would help, as would any reduction in 
the size of each style context instance.

(Using the Show Style Size command from Viewer the number and sizes of the 
contexts can be seen for any page or site.)

Comment 1

[Marc Attinasi]

I have made a change (not checked in yet) to allow Style Context's to share 
common style data across the board. This results in a 40% reduction in the 
amount of memory taken up by the style system for large pages. There are some 
risks and further testing is needed, but the approach seems sound (and was 
discussed with Pierre and Troy).

Further size reduction in the Style Contexts will probably require a more 
comprehensive re-engineering of how style data is maintained in memory...

Comment 2

[Marc Attinasi]

The changes I have made for reducing style context memory usage by sharing style 
data has been tested now by myself for several weeks. I am nominating it for 
nsbeta2 because I feel that if we do not get it in for the beta it will probably 
never get into the release. This is a major win in terms or performance, 
reducing memory footprint substantially, especially on very complex sites. The 
code can be turned ON and OFF with a #define (compile-time). For safety, we can 
even add a pref to disable it when compiled in.

There are some risks associated with this change, especially for DHTML 
documents, however the benefits are substantial and if we are going to consider 
taking this change for the release we need to get some wide-spread testing on 
it.

An overview of the design is at 
http://techno.mcom.com/users/attinasi/publish/StyleContextDataSharing.html

Comment 3

[Peter Trudelle]

[nsbeta2+]

Comment 4

[Marc Attinasi]

Pending code review...

Comment 5

[Marc Attinasi]

Chris Waterson was kind enough to review the code: it is ready to go now. There 
will be some additional work later to incrementally improve the performance of 
the implementation (use of Arena for allocations, more memory friendly Hash 
Table) but that will get its own bug.

Comment 6

[Marc Attinasi]

Attachment: Attached a snapshot of the document since it is posted inside the firewall. Note that it may be out of date...

Comment 7

[Marc Attinasi]

Updating milestone. This one will be landing very shortly - waiting for stable 
tree...

Comment 8

[rbs]

Marc, what about putting this valuable material on the mozilla.org website?
This is good stuff, especially considering that more and more people (e.g.,
embedding) are getting concerned about memory consumption. How does the 'global
sharing' of the nsStyleContextData interacts with the former 'local sharing' of
nsStyleContexts between siblings? Are both still present? Or is the maintainance
of the later not worth the effort anymore?

Comment 9

[Marc Attinasi]

The sharing of sibling contexts still exists, in addition to global sharing of 
context data. I didn't see any reason to take away the existing style context 
sharing, especially since it is actually better to share the entire context then 
just the style context data (you save an extra 36 bytes due to the 
parent-sibling pointers).

I'll look into getting the Style Context Sharing doc on Mozilla.org - I have 
some other style docs I'd like to get up there too, but they need to be 
completed (the style system is largely undocumented).

Comment 10

[Marc Attinasi]

Fix in: nsIStyleContext.h, nsIStyleSet.h, nsStyleContext.cpp, nsStyleSet.cpp

Use viewer's Show Style Size to verify that style size used is significantly 
lower (30-40% depending on the page, the larger the page the more the benefit).

Comment 11

[Adam D. Moss]

Excellent.  Does this share context data across
a single document or globally?  I'm thinking
that it could reduce suckage significantly
for people who like to spawn multiple browser
windows with documents from a single site (and
hence in liklihood with similar style rules).

Comment 12

[Marc Attinasi]

Adam, right now style data is shared across a document, so I guess it is not 
really as 'global' as I might have infered. The root of the sharing is in the 
StyleSet and that is maintained per PresShell (and there is a pres shell per 
document instance I believe). Conceptually the style data could be shared across 
all style sets, and that is something that is worth trying. I can easily 
envision hanging the style context cache off of a singleton-service instead of 
the style set... I cannot think of any technical reasons to restrict style 
context sharing to a single document. I'll add this excellent idea to my list of 
future improvements to style context sharing (I think it is time to open a 
tracking bug for this topic.)

(It seems to me we could share the style rules across style sets as well. I'll 
have to look into how that currently works. Now you have got me thinking...)

Comment 13

[Marc Attinasi]

Created bug 43457 to track style data sharing improvement ideas. Please put 
additional improvement ideas in bug 43457.

Comment 14

[Marc Attinasi]

Reopened due to temporarily turning off sharing while XUL problems are 
investigated.

Comment 15

[rbs]

Adding dependency to XUL problem (bug 43490) for ease of reference.

Comment 16

[Marc Attinasi]

This was provided by David Baron:

The following bugs seem to have been fixed when you turned off style
context sharing, so you should check them before turning it back on:
43942
43685
44251
44325

If you confirm that that was the problem, probably you should assign
them to yourself (assuming you plan to turn it back on sometime).

Comment 17

[Marc Attinasi]

Here is another I need to verify when style context sharing is turned back on - 
Form Submission when a text input has :focus rule - bug 43988

Comment 18

[Marc Attinasi]

I want to de-nominate this from nsbeta2. The change has shown many small 
problems in XUL that I cannot yet explain, and the risk is simply too high. The 
changes have already been disabled due to reported bugs.

Comment 19

[rbs]

The caching technique, as described, is sound to me (I only see that it may 
sligthly affect speed). I will be curious to know why it is failing when you
revisit the issue.

Failure of the caching seems to suggest that some style subtrees may be
corrupted (perhaps due to the usage of the deprecated methods that are still in
nsIStyleContext).

Comment 20

[Marc Attinasi]

Yes, GetMutableStyle calls are suspect, however they are not the only problem. 
Turning on style tree verification shows many, many style tree complaints coming 
from illegal parent contexts related to XBL frames, cases where the style tree 
does not match the frame tree. Unfortunately I cannot determine just *why* the 
style tree needs to match the frame tree, so I cannot determine how much of a 
problem that is. Since all of the visible problems were in XUL/XBL elements, I 
am guessing the problems are related, but it is just a guess at this point.

Comment 21

[rbs]

I remember to have seen peterl insisting that frame owners honor this match.
(news://news.mozilla.org/37E73E09.BC12390E%40netscape.com)
It probably has something to do with dynamic updates. For example, if child
frames are shuffled around, the Style System can update their style contexts
relatively quickly. If the assumption that the two trees match is removed,
I guess it will be tough (and very slow) for the Style System to keep up with
dynamic updates.

Comment 22

[Marc Attinasi]

Some more information: I added code to GetMutableStyle to cause a unique 
instance of the style data to be created (since the caller is probably going to 
change it) and the behavior was worse. So I then turned off sharing again, and 
used the same new code to simply create a new instance of the style data for the 
context, copying the values from the old data. The end result then is a context 
with a new style data instance that has exactly the same data as the old style 
data instance, but it is a new instance. This exhibits the same problematic 
behavior. Either the copy of the data is not correct (but it is checked after 
the copy to ensure all fields are identical using existing CalcDifference calls) 
or somebody is holding on to old style data structures which now probably 
contain garbage. This could cause a lot of trouble depending upon how long 
clients are holding onto style data structures, and what they are using them 
for... I'm going to look further into who might be caching style data and for 
how long. If anybody has any clues please do tell!

Comment 23

[Pierre Saslawsky]

CCing more folks and repeating Marc's question a bit louder... :-)
WHO IS HOLDING TO THE DATA STRUCTURES AFTER CALLING GetMutableStyleData()?

Comment 24

[David Hyatt]

Reading peter linss' news post, I can say that XBL violates requirement #2 on 
his list.  He essentially stated that the style context trees and the frame 
trees should match.  In the case of XBL, they do not.  Perhaps we should 
investigate what would be involved in relaxing this restriction, since XBL 
works fine currently (the complaints in verification are only being spit out 
because the code assumes the style tree and frame tree match, which they don't
when you use XBL's <children> feature).

Comment 25

[rbs]

Some thoughts:

ASSUMPTION: match of frame and style trees
===========
cons: 

  * burden on frame owners (a one-time effort to get their frame code right)

  * more?

pros:

  * the Style System can do all its space optimizations in peace, such as:
    - local sharing between siblings (this buys a lot when the
      contents of the nodes is in fact the same all over a subtree) 
    - Marc's ongoing work to do global sharing over the entire style tree
      and even across separate documents

  * the Style System can do all its speed optimizations in peace, such as:
    - a (dynamic) change somewhere means that the ReResolveStyleContext
      logic in the nsFrameManager only has to grovel the corresponding
      subtree

  * more?


ASSUMPTION: no match of frame and style trees
===========
pros:
  * frame owners don't have to worry anymore about the present constraints

  * the Style System can do its space optimizations as it pleases

  * more?

cons: 

  * burden on frame owners and the Style System: the current ownership model has
    to be redesigned because a straight mStyleContext can point to something
    this time, and to something else next time if the data has changed in the
    meantime. To retrieve the style data associated to a frame, the frame will
    now have to be passed as parameter to a method of the Style System which
    will have to maintain a kinda mapping, and guarantee the coherence and the
    integrity of this framework.

  * gone are the nifty things that frame owners have been doing because
    they *knew* that there was a mirroring of style<->frame trees.

  * speed-down in the Style System: a (dynamic) change somewhere will mean that
    the ReResolveStyleContext logic in the frame manager would have to grovel a 
    large style tree, a process which can entail the creation more style nodes
    (because existing nodes would have to be kept for the other owners). For any
    such change, the Style System will have to update its one (context) -to-
    many (frames) mapping.

  * more?

Comment 26

[rbs]

My vote: retain the mirroring of style<->frame trees, and make clients
compliant by:
* honoring the match (nsbeta3 for this one? to allow Marc's perf enh.?) 
* eliminating the use of deprecated methods (GetMutableStyleData and friends).
It is not a simple task, but the other option of changing the matching
assumption would significantly impact on the Style System and would still mean
that frame owners would have to adapt their code to the new model.

So we can leave a revamp of the Style System for later. It could be that
an overhaul will be needed for the more important job of implementing
extensible (optional/pluggable) CSS3 modules.

Comment 27

[David Hyatt]

I don't have the first clue how to patch XBL to keep the style tree in sync
with the frame tree.  This feature of XBL cannot be removed without destroying 
skinnability (e.g., the Classic skin would not be possible).

Comment 28

[rbs]

Do you mind elaborating on the details of the problem and giving us a pointer to
the source. Is the problem that you want a change in a parent style context to
be seen in the <children> frames? If that's the case, wouldn't that be possible
to create the child frames with their own contexts (or pseudo-contexts), and use
some API of the style system to propagate any change from the parent? This way,
the Style System will be aware of the changes and do the necessary work re:
keeping its caching mechanism in sync?

Or is the problem more complex in that you want the changes to be seen only by 
the first (or up to the n-th) generation of <children> or something like that?

Comment 29

[Marc Attinasi]

I'll throw this in because karnaze is not here: Tables setup their contexts out 
of synch with the frame tree (the TableInner and TableOuter frames have their 
contexts in reverse parentage). Chris told me it would me much harder to 
implement the other (correct) way, and at that time I did not know about the 
VerifyStyleTree routine and associated rules.

I am still looking for specific assumptions about the frame tree matching the 
style tree hierarchy in the style code so I can better understand the cost of 
removing those assumptions. Although Peter was very clear about what a clean 
style tree should look like, he did not indicate what would happen when the 
style tree was not clean (other than suggest that the violations he did know 
about were not beta-stoppers).

Obviously the style data sharing scheme is not going to get into first release 
of SeaMonkey. Since there appear to be no problems in Viewer viewing HTML files 
and even individual XUL files, maybe there is still hope for the embedding folks 
to use it, although it is a risk for them to use code that is not well tested in 
the browser. For the next release we are likely to need and want a more 
extensive revamp of style contexts - the sharing idea was really jsut an interim 
'low impact' way to achieve some pretty good footprint reduction.

Comment 30

[Marc Attinasi]

It looks like bug 45210 is showing a case where having the style tree *not8 
match the frame tree (in terms of parentage) is causing problems. It is realted 
to how ReResolveStyleContext recurses over child frames. I'm still digging, but 
it smells like one of the assumptions I was looking for. Anybody else wanna 
validate or refute my thinking on that?

Comment 31

[David Hyatt]

Marc, maybe I could keep the style trees in sync with the frame trees.  Here's 
what I need to keep working.

Given two content nodes A and B, XBL lets you interleave anonymous content 
(we'll call it C).  This means that my frame tree looks like this...

A --> (C) --> B

I end up building a frame for B and then placing it as a child of C.  However, I 
do not reparent the style context when I do this, so the style context tree 
looks like this...

A --> (C)
  --> B

Now I did this because I wanted to ensure that style rules of the form

A > B {
  la la la
}

continued to work.  If you can guarantee to me that even with a style tree of 

A --> (C) --> B

that the style system will still match 

A > B 

correctly, then I could make this switch in XBL.

Comment 32

[Marc Attinasi]

David, I'm almost 100% positive that inserting anonymous content will not change 
style rules. I know we have cases of anonymous content now and I'd like to dig 
into how that works to make sure that I am not missing something, but from my 
understanding or how anonymous content works, and how selectors are matched, 
there should be no reason to worry about the anonymous content skrewing up the 
style rules.

Comment 33

[David Hyatt]

Ok, then I think we're gonna be ok here.  How do I turn on your optimizations?  
I'll try producing an XBL patch and see if it works.

Comment 34

[Marc Attinasi]

The style data sharing is turned on in one of two ways:
1) Change the initialization of the static variable 'bEnableSharing' to PR_TRUE 
in nsStyleContext.cpp (line 2846) and recompile.
2) In the debugger, change the value of bEnableSharing to 1 on the first access 
(nsStyleContext.cpp line 2849). This is very useful for doing comparisons 
without recompiling.

The most common places we saw problems were in the location bar, tooltips, and 
the preferences dialogs. In all cases the background colors were wrong on some 
elements (generally reverting to the default) and the fonts sometimes got 
whacked too.

Of course enabling of the styletree verification is done via the 
NSPR_LOG_MODULES env. var. - set to styleverifytree:1 before you execute mozilla

David, if you wanna supply me a patch I'd be more than happy to pound on it as 
well. Now I've gotta get Karnaze to re-work the table contexts too... He is less 
eager than you ;\

Comment 35

[Pierre Saslawsky]

David, I confirm that the "A > B" rules will continue to work (dbaron fixed the 
last remaining problems 3 months ago in bug 24031) but if you have some "A B" 
rules, they may break.

Comment 36

[David Hyatt]

Ok, boys.  Here's the patch. Let me know if XUL starts behaving with this.

Comment 37

[David Hyatt]

Attachment: XBL properly reparents the style contexts of any explicit frames that it moves

Comment 38

[Marc Attinasi]

Thanks soooo much for the patch, Hyatt. Unfortunately we are still having 
problems with text fields in dialogs and on the location bar. I'll look deeper 
and see what is going on there. The warnings from teh VerifyStyleTree logging 
are drastically reduced by your change, so I think it is a good once even if it 
does not close the chapter on this bug.

Comment 39

[David Hyatt]

We could look at this when you come up if you want.

Comment 40

[Marc Attinasi]

Moving all non-nsbeta3 bugs to future milestone: these will be worked on after 
beta3/rtm.

Comment 41

[Daniel Bratell]

Is there any special reason this isn't nominated for nsbeta3? If i remember 
correctly it shaved of a measurable chunk of bloat when the first version was 
tested.

Comment 42

[Marc Attinasi]

There are too many problems with the way style contexts are used to sneak this 
in for beta3. Yes, it provides a dramatic improvement in bloat, but it wreaks 
havoc on the chrome (see the list of bugs that were fixed byt turning this 
off in the comments above). The code is all checked in, it is just turned OFF by 
a static variable bEnableSharing in nsStyleContext.cpp, in case you want to play 
with it.

Comment 43

[Marc Attinasi]

It looks like we have fixed most of the style context problems now, but not
wuite all. Setting the environment variable NSPR_LOG_MODULES=styleverifytree:1
causes diagnostic style context parentage check to be run, and violations fo the
style tree rules are output to the console. We had lots of violations, but they
are mostly fixed. Turning on sharing now the chrome looks pretty good, with no
flagrant problems except for the color-pickers showing the link and page color
in the prefs dialog area. I'll have to check those out more closely. Also,
Classic and Modern chromes need to both be tested. Still, tables are a big
violator of the context parentage rules, so that needs to be fixed before the
sharing is enabled.

Comment 44

[David Hyatt]

Exciting news.  I did find another code path by which XBL could not reparent 
style contexts to match the frame tree.  I may need help in coming up with a 
patch though.  Let me look into it some more.

Comment 45

[Marc Attinasi]

milestone set

Comment 46

[Marc Attinasi]

I'm about ready to land this again, with some minor changes. After lots of
experimenting and investigation it is now clear that sharing of style context
data works fine for all encountered frames except for the GfxScrollFrame. I
cannot understand why there are problems with the scroll frame, but I am
satisfied that simply excluding the scroll frame from sharing is sufficient, at
least in the near-term. Long term I think we need to understand what is going on
in there and either fix of accomodate the scroll frame's behavior.

I'll attach a patch with the changes to the style data sharing code
(nsStyleContext.cpp). Pierre has reviewd it, and I'm now seeking approval for
the checkin.

Oh yea, I added the ability to turn off the sharing by an environment variable
to make it possible to do comparison metrics without having to rebuild or
execute in the debugger.

Comment 47

[Marc Attinasi]

Attachment: Patch to enable sharing (for all frames except ScrollFrame)

Comment 48

[Marc Attinasi]

A couple of more notes about the sharing changes: I have been testing with them
for about a week now and the old problems are not apparent. Additionally, I have
run the new style regression tests and they show that the style data is
consistent. There is the 10% slowdown in style resolution that I saw before, and
the gain still seems to be from 10% - 40% depending on the page. Hopefully when
we address bug 43457 we can improve the performance since a 10% slow down is not
great.

Comment 49

[David Hyatt]

sr=hyatt.  let me know if you see any oddities with XUL elements that use
complex XBL.  I think I'm properly reparenting in all cases now, but something
may slip through the cracks.

Comment 50

[Marc Attinasi]

The style data sata sharing code has been enabled again (with modifications as 
previously described). nsStyleContext.cpp

Refinements / improvements / replacements to give better performance or better 
savings should be reported in bug 43457. This one should only be reopened if we 
have to turn off the sharing, heaven forbid - thanks.

Comment 51

[Marc Attinasi]

Here is a tasty informational biscuit for ya:

To turn off the style context data sharing, set an environment variable like 
'moz_disable_style_sharing=1' - very handy for testing for regressions.

Comment 52

[Hixie (not reading bugmail)]

Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...

Comment 53

[Hixie (not reading bugmail)]

rubber stamp verify