The CSS stylesheet data structures need to go on a diet. They take up a good bit of memory (at least I've certainly seen that in the past, based on trace-malloc type output). They're also allocated in small chunks and therefore allocating them and freeing them takes up a good bit of time (e.g., much of the time that shows up within nsCSSParser::Parse is really allocation of the sheet data structures). Some of the structures probably also use too much COM -- we can use more wrappers for DOM access if we need to. The performance costs for destruction in a window-close profile seem to be mostly in structures such as nsCSSMargin, nsCSSDeclaration, and nsCSSRule. However, the performance costs in allocating during parsing seem to be more for the internal selector data structures and the growth of arrays although the data structures also show up a bit. We should probably profile stylesheet construction and destruction in more detail before delving into this.

A related bug is bug 106356 (reduce the number of memory allocation in CSS parser), and maybe bug 91242 too (CSS parsing is 5.5% of startup time).

Since this bug was originally filed as basically the same thing sa bug 106356, I'm going to make it a dependent bug and change it to cover one part of that problem -- the size of CSS declarations and the data structures that they own. I think reducing the size of rules and selectors should probably be a separate bug, although things may end up being too intertwined. Right now, the amount we allocate for the declaration block (the CSS term for what we call an CSSDeclarationImpl) is ridiculous. To represent the declaration block: { font-weight: bold; } We allocate: 1 CSSDeclarationImpl, 19 words (vtable pointer, refcount, 14 data struct pointers, a void array pointer, and a string array pointer) 1 nsCSSFont, 15 words (vtable pointer, 7 nsCSSValue objects at 2 words each) 1 nsAutoVoidArray, 12 words So this declaration is actually smaller when stored as a text string with all whitespace (25 bytes as char*, 50 bytes as PRUnichar*) than it is in its "compiled" form (184 bytes, plus the malloc overhead of 3 separate allocations). A solution I've been thinking about is to try to allocate the nsCSSDeclaration (I'd like to see if we can get rid of the COM layer here, perhaps with a wrapper for script) and the things that it owns in a single block. I've only spent an hour or so thinking about this so far, so this plan is pretty vague, but what I've been thinking is this: When the CSS parser parses a declaration, it could parse into a data structure much like what we currently have as a CSSDeclarationImpl with all its structs, allocated on the stack. We could then look through this structure and allocate a data structure that looks something like the following (note that the memory of comments and of order is gone -- we don't need either to comply with the CSSOM spec, and I don't think we should have either), all within a single block of memory: PRUint8 numStructs; /* or maybe PRUint8 for alignment */ then, for each "struct" that we need: { PRUint8 structID : 4; PRUint16 offset : 12; /* from beginning of declaration */ } then have each struct (in the same order as the IDs), have a similar structure: PRUint8 numProps; for each property: PRUint8 propID; /* no offset since we can look up the size of each property in a table */ then have each property's value, in the same order With a little casting magic we could probably even make these things look like regular C++ classes to the caller.

in addition to PRUint8 numStructs; /* or maybe PRUint8 for alignment */ I think it would also be very helpful for performance to have PRUint32 structBits; /* set for each struct that we have */ so that we can skip declarations much faster when walking the rule tree. I had also been thinking that it might not be a good idea to have all the separate struct concepts, but now I'm again thinking that this is probably good.

Garrett is probably going to work on this.

A few more thoughts here: * The current CSSDeclarationImpl class stores the order of the declarations and stores comments. Neither of these is really necessary according to the CSSOM, and I think we should drop them due to the amount of memory they take up. * I somehow doubt that there's any good reason for nsICSSDeclaration to be an interface -- if it makes sense for anything to be an interface, it would be the rules, not the declarations (although perhaps some sort of DOM wrapper on the rules that doesn't have multiple instances when there are multiple selectors)

Footprint/performance numbers to follow later today.

One additional note, mOrder of nsCSSDelcaration has various forms of access through member functions that spread out into different modules. I was not confident enough to completely remove this member, though I did decrease the impact of it on the heap. Someone much more familiar than I should make the call on mOrder. That or it will be a while before I am confident enough to make the call myself.

build on linux....

Page load performance results from cowtools/page-loader under win32: Before: 479 msec After: 474 msec Increase in page load performance of 0.84%. In other words, no speed penalty for proposed changes. Footprint results to follow soon.

Footprint resulting from CSS Declarations on Startup, about:blank, Shutdown (modern theme): Before (filtered on CSSDeclarationImpl): 276,304 bytes. After (filtered on nsCSSDeclaration): 214,560 bytes. 61,744 bytes saved on startup. A 22.35% footprint reduction of CSS declarations. Numbers produced using spacetrace.

whoopy! that's awesome! :-)

Comment on attachment 62770 [details] [diff] [review] reduce CSSDeclaration footprint request for r=,sr=

Well, this patch only attacks the bloat in the CSS declaration itself and not the bloat in the structs, but it seems to be a reasonable start. I like the de-comification, although I'm not sure whether the rest of the patch would hold up if we wanted to fully do what I originally proposed. Do you have any idea how much of the footprint reduction came from the removal of |mComments| and how much came from the restructuring of the struct storage? It would probably be nice (in another patch, most likely) to change the SIDs of the rule structs to be smaller (not 128-bit) and perhaps also reorganize the rule structs so they match the data structs (and their IDs as well). + mOrder = /* CSS_NEW_NSVALUEARRAY */ new nsValueArray((nsCSSProperty)eCSSProperty_COUNT, 8, aCopy.mOrder->Count()); 80th-column violation It might be better to rewrite nsCSSDeclaration::GetData and nsCSSDeclaration::EnsureData to use early returns for each struct rather than a long if-else chain. In your new |CSS_ENSURE| macro, I don't see the need for the additional variable |newMe|. -NS_IMETHODIMP -CSSDeclarationImpl::GetValue(const nsAReadableString& aProperty, +nsresult +nsCSSDeclaration::GetValue(const nsAReadableString& aProperty, nsAWritableString& aValue) That leaves bad indentation of the second line (and same thing a few lines below for the other |GetValue|). PRBool -CSSDeclarationImpl::AllPropertiesSameValue(PRInt32 aFirst, PRInt32 aSecond, +nsCSSDeclaration::AllPropertiesSameValue(PRInt32 aFirst, PRInt32 aSecond, PRInt32 aThird, PRInt32 aFourth) Ditto. And same thing in a bunch more places which I'll stop mentioning every time I notice one. nsCSSDeclaration::ToString(nsAWritableString& aString) { - if (nsnull != mOrder) { + if(nsnull != mOrder) { PRInt32 count = mOrder->Count(); Ugh! In nsCSSDeclaration.h: + // Specialized ref counting. + // We do not want everyone to ref count us, only the rules which hold + // onto us (our well defined lifetime is when the last rule releases + // us). It's worth a comment here that the main nsCSSDeclaration is refcounted, but it's |mImportant| is not refcounted, but just owned by the non-important declaration. It might also be nice to call these methods |AddRef| and |Release| so they can be used with |nsRefPtr| once I finish it. +// +// static +// moved here to avoid linkage dependencies in content/base on js, et. al. +// +nsUniqueStyleItems *nsUniqueStyleItems ::mInstance = nsnull; + Doesn't putting it in content/shared create the potential for two instances (one in content and one in layout)? Shouldn't it just stay in content only? +// maximum value was 1275, then 1 word (uint16) will represent each value +// in the array instead of 2 words (uint32). Isn't that Win16 terminology? nsValueArray seems to have too many inline members. It seems to me like this would expand to lots of code, and you'd be better off with most of them (the ones that can't be written in one line) non-inline. (RemoveValue could be written in one line, though.) Should the growth strategy of nsValueArray be different? Doesn't the current approach malloc too much when growing to a large size? What does nsVoidArray do?

dbaron wrote: >Should the growth strategy of nsValueArray be different? Doesn't the >current approach malloc too much when growing to a large size? What >does nsVoidArray do? Binary exponential is necessary above some threshold, or you'll get O(n**2) growth of realloc/copy costs, and horrible heap fragmentation. /be

dbaron's great review said: > Well, this patch only attacks the bloat in the CSS declaration itself > and not the bloat in the structs, but it seems to be a reasonable start. > I like the de-comification, although I'm not sure whether the rest of > the patch would hold up if we wanted to fully do what I originally > proposed. What did you have in mind so it will be tracked in the future? > Do you have any idea how much of the footprint reduction came from the > removal of |mComments| and how much came from the restructuring of the > struct storage? No idea. The comments above indicating mComments was not needed was enough to prompt the removal. > +// maximum value was 1275, then 1 word (uint16) will represent each value > +// in the array instead of 2 words (uint32). > > Isn't that Win16 terminology? Heh, not win16 terms to my knowledge. In any event, I will use bytes instead of words to avoid contention if that is the basis of the question. The remainder of the review feedback should be integrated into the coming patch.

new patch after review

explicitly requesting r= sr=

Comment on attachment 64165 [details] [diff] [review] reduce CSSDeclaration footprint r=dbaron

Comment on attachment 64165 [details] [diff] [review] reduce CSSDeclaration footprint sr=hyatt

Checked in patch, closing bug.

Just wanting to make one important comment about comments in CSSOM... If we ever add Stylesheet creation/manipulation to Composer, we will need those comments. It is not acceptable from a web author's point of view to have his comments stripped. Comments are not stripped from Markup when we load and save a document in Composer and we should reach the same level of quality for stylesheets. No flames in answer to this please...

The idea could be keeping the comments only for stylesheets other than UA stylesheets and chrome stylesheets.

>No flames in answer to this please... No flames at all. I remember Master Linss explaining why we had to save comments in stylesheets for that very purpose of round-tripping of documents.

Have to, even though it's a huge waste of memory 99.99% of the time?

It depends on the application. In a browser, it is a waste of memory 100% of the time but in a CSS-aware editor, it becomes a requirement. We could store the comments by default but in Navigator trigger a mode where we don't store them at all.

Well, even in a browser, a document can contain script looking at the comments in the stylesheet and do something depending on the presence, the content... A content editor can look at the html *and* css comments to correctly open a document authored by Dream*eaver... That's allowed by the DOM spec and the CSS spec, and I was under the impression that one major goal of Mozilla is compliance to standards. Once again, we keep markup comments in the tree ; we should also keep in the tree css comments coming from outside of the chrome.

The CSSOM (DOM 2 Style) requires neither that we keep comments nor that we remember the order of declarations within a declaration-block.

Please, don't try to teach bureaucracy to a french citizen... :-) The way comments are specified in DOM 2 Style are one of the major comments all the editing tool vendors made about this specification. DOM 2 Style has been designed only with browsers in mind, totally forgetting that authoring tools and filters may also have to use it. Selectors for instance are kept in string form, which is a pure non-sense from a DOM point of view. Because of that, it is impossible to correctly store CSS comments in the DOM. where do you attach a comment found between two tokens ? And to which rule should be attached a comment between two rules ? What about two adjacent comments ? Merged or not ? Comments are needed in the 0.01% cases you omit to deal with. That's factual. And I am trying to improve CSSOM with that in mind.