reduce size of CSSDeclaration and things it owns (nsCSSMargin, etc.)

RESOLVED FIXED in mozilla0.9.8

Status

()

RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: dbaron, Assigned: blythe)

Tracking

Trunk
mozilla0.9.8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.

Comment 1

17 years ago
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.
Blocks: 106356
Summary: stylesheet data structures need a diet → reduce size of CSSDeclaration and things it owns (nsCSSMargin, etc.)
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.
Assignee: dbaron → blythe
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)
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

17 years ago
Created attachment 62761 [details] [diff] [review]
reduce CSSDeclaration footprint

Footprint/performance numbers to follow later today.
(Assignee)

Comment 7

17 years ago
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.
(Assignee)

Comment 8

17 years ago
Created attachment 62770 [details] [diff] [review]
reduce CSSDeclaration footprint

build on linux....
Attachment #62761 - Attachment is obsolete: true
(Assignee)

Comment 9

17 years ago
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.
(Assignee)

Comment 10

17 years ago
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.

Comment 11

17 years ago
whoopy!  that's awesome!  :-)
(Assignee)

Comment 12

17 years ago
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

Updated

17 years ago
Target Milestone: --- → mozilla0.9.8
(Assignee)

Comment 15

17 years ago
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.
(Assignee)

Comment 16

17 years ago
Created attachment 64165 [details] [diff] [review]
reduce CSSDeclaration footprint

new patch after review
Attachment #62770 - Attachment is obsolete: true
(Assignee)

Comment 17

17 years ago
explicitly requesting r= sr=
Comment on attachment 64165 [details] [diff] [review]
reduce CSSDeclaration footprint

r=dbaron
Attachment #64165 - Flags: review+

Comment 19

17 years ago
Comment on attachment 64165 [details] [diff] [review]
reduce CSSDeclaration footprint

sr=hyatt
Attachment #64165 - Flags: superreview+
(Assignee)

Comment 20

17 years ago
Checked in patch, closing bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
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.

Comment 23

17 years ago
>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?

Comment 25

17 years ago
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.

Updated

17 years ago
Blocks: 92580
You need to log in before you can comment on or make changes to this bug.