Closed
Bug 39618
Opened 25 years ago
Closed 24 years ago
Style Contexts are taking up way too much memory
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla0.8
People
(Reporter: attinasi, Assigned: attinasi)
References
()
Details
(Keywords: embed, memory-footprint)
Attachments
(3 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•25 years ago
|
||
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...
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•25 years ago
|
||
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
Keywords: nsbeta2
Assignee | ||
Comment 5•24 years ago
|
||
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.
Assignee | ||
Comment 6•24 years ago
|
||
Assignee | ||
Comment 7•24 years ago
|
||
Updating milestone. This one will be landing very shortly - waiting for stable
tree...
Target Milestone: M16 → M17
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?
Assignee | ||
Comment 9•24 years ago
|
||
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).
Assignee | ||
Comment 10•24 years ago
|
||
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).
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 11•24 years ago
|
||
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).
Assignee | ||
Comment 12•24 years ago
|
||
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...)
Assignee | ||
Comment 13•24 years ago
|
||
Assignee | ||
Comment 14•24 years ago
|
||
Reopened due to temporarily turning off sharing while XUL problems are
investigated.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 15•24 years ago
|
||
Adding dependency to XUL problem (bug 43490) for ease of reference.
Blocks: 43490
Assignee | ||
Comment 16•24 years ago
|
||
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).
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 17•24 years ago
|
||
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
Assignee | ||
Comment 18•24 years ago
|
||
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.
Keywords: nsbeta2
Whiteboard: [nsbeta2+]
Comment 19•24 years ago
|
||
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).
Assignee | ||
Comment 20•24 years ago
|
||
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•24 years ago
|
||
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.
Assignee | ||
Comment 22•24 years ago
|
||
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•24 years ago
|
||
CCing more folks and repeating Marc's question a bit louder... :-)
WHO IS HOLDING TO THE DATA STRUCTURES AFTER CALLING GetMutableStyleData()?
Comment 24•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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?
Assignee | ||
Comment 29•24 years ago
|
||
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.
Assignee | ||
Comment 30•24 years ago
|
||
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•24 years ago
|
||
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.
Assignee | ||
Comment 32•24 years ago
|
||
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•24 years ago
|
||
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.
Assignee | ||
Comment 34•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
Ok, boys. Here's the patch. Let me know if XUL starts behaving with this.
Comment 37•24 years ago
|
||
Assignee | ||
Comment 38•24 years ago
|
||
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.
Target Milestone: M17 → M18
Comment 39•24 years ago
|
||
We could look at this when you come up if you want.
Assignee | ||
Comment 40•24 years ago
|
||
Moving all non-nsbeta3 bugs to future milestone: these will be worked on after
beta3/rtm.
Target Milestone: M18 → Future
Comment 41•24 years ago
|
||
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.
Assignee | ||
Comment 42•24 years ago
|
||
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.
Assignee | ||
Comment 43•24 years ago
|
||
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•24 years ago
|
||
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.
Assignee | ||
Comment 46•24 years ago
|
||
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.
Assignee | ||
Comment 47•24 years ago
|
||
Assignee | ||
Comment 48•24 years ago
|
||
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•24 years ago
|
||
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.
Assignee | ||
Comment 50•24 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 51•24 years ago
|
||
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•24 years ago
|
||
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...
QA Contact: chrisd → ian
You need to log in
before you can comment on or make changes to this bug.
Description
•