42953 - too much string appending in CSS Parser

Status: RESOLVED WONTFIX

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I've heard comments that the CSS parser does too much string appending (and it 
sure looks this way :-/.   **If this is true**, I think there's a relatively 
easy solution.

The reason it does so much appending to strings is that the CSSParser builds up 
a string representing the selector text one token at a time.  It would be much 
simpler, I think, if the tokenizer were able to store the buffer that it was 
tokenizing, which could then be retrieved by the parser if it wanted to know the 
text that it parsed since a certain point.

What I'm not sure of is whether the parser would ever want the scanner to be 
remembering two different things at once.  If not, the scanner could have 
methods such as |StartMemory|, |GetMemory|, and |ClearMemory|.  The memory could 
initially be just a mark in the buffer (currently 256 chars), but when the 
buffer was refreshed it would have to be a string storing old buffers.

A solution like this would probably make it easier to implement some of the 
cssText properties (that currently look unimplemented in nsCSSStyleRule.cpp.
It would hopefully allow nsCSSToken::AppendToString to be removed.

Does this sound reasonable? (assuming this is a real performance problem)

Comment 1

[Marc Attinasi]

This sounds reasonable to me. Using a string class for building up tokens seems 
reasonable too, provided the string class is efficient at appending. Is the 
problem that our strings are lousy when it comes to appending? Could we just set 
the capacity of the strings in advance and thus avoid the inevitable reallocs 
that result from growing strings char by char? That seems like a simpler change, 
provided our strings still support it. 

Has anyone seen string appends in the CSSParser showing up in quantify?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

When scc had the string changes in that made the allocation different, it was a 
performance problem.  It also seems to be a good percentage of the work the CSS 
parser is doing.  Harish also mentioned to me earlier that he had seen 
performance problems in the CSS parser (at some other time).  I'll ask him for 
more detail...

Comment 3

[Pierre Saslawsky]

If Harish did some performance analysis before the new string classes landed, it 
is probably obsolete by now. I think it's a better idea to do it all over again, 
compare with the numbers that Harish obtained previously, and if there is still a 
problem, gather some details and ask Scott Collins how he can help us to help 
ourselves. Maybe he will modify the string classes for the benefit of everybody, 
or just tell you how to use the existing classes more efficiently in the parser. 
Get him involved anyhow (and tell him I sent you :-).

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I also wonder if these strings are causing bloat.  I saw a good bit of bloat
caused by CSSOM structures in one of beard's recent GC-generated bloat logs.  I
have the feeling they are built for CSSOM manipulation of the cssText attribute
(which may only work for some things anyway -- I'm not sure).  Is there any need
for these strings at all?

Assigning this to Marc since I'm not going to have time to work on this.  I'm
not really sure if it's serious or not.  Frankly, the bloat issue may be a
bigger issue than the performance, but I'm just not sure...

For the record, I did email scc, but never heard back.

Comment 5

[Marc Attinasi]

Moving to Future until told otherwise...

To move this in from the Future, nominate for beta3 and explain why it is 
important enought for beta3/RTM of Netscape6. The powers that be will approve or 
deny it.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Perhaps the best solution here is to generate the CSSText properties dynamically 

and get rid of all this string stuff in the CSS parser.

Comment 7

[Marc Attinasi]

Pulling in from Future: performance work is back on the radar...

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hewitt recently checked in some changes to generate the cssText for selectors
dynamically (since it wasn't generated at all before).  I don't think there's
too much work left to generate it all dynamically and get rid of all the
source storage.

Comment 9

[Marc Attinasi]

Moving out to Mozilla 1.0 milestone - small benefit in terms of performance and 
footprint.

Comment 10

[ckritzer (gone)]

Moving to correct QA Contact- here ya go Ian!

Comment 11

[Hixie (not reading bugmail)]

code level bug -> dbaron

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Daniel Glazman fixed part of this problem as bug 70995.

Comment 13

[Kevin McCluskey (gone)]

Reassigning to Pierre. 

Note: Marc's comment "small benefit in terms of performance and footprint", so
I'm setting milestone to future.

Comment 14

[Pierre Saslawsky]

First we must check whether there is still something to improve.  Boris, do you 
want to look into it?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Assigning pierre's remaining Style System-related bugs to myself.

Comment 16

[Boris Zbarsky [:bzbarsky]]

So what are the outstanding issues here?

The scanner is still doing some appending to tokens, but those are using
nsAutoString for the mIdent, so for the most part that should be fine.  The one
exception I can see is comments... The part of nsCSSScanner::Next that calls
SetCapacity(2) on the comment token's mIdent is basically a no-op (we should
remove that, methinks).

I have to wonder whether we could win anything by not even creating comment
tokens....  That would certainly save us having to append the license comments
in the chrome CSS (which are over 80 chars long, so probably cause lots of
reallocs).

Anything else that misuses strings that I'm missing?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: jprof profile of CSS parsing on http://www.people.fas.harvard.edu/~dbaron/home

This profile still shows a good bit of time spent doing string appending.

Comment 18

[Alfred Kayser]

As said in comment 16, the string handling is now done using nsAutoString, and furthermore the comments are now skipped (SkipCComment), so they are no longer a performance issue. See also bug 60290, where the comment skipping was realised.

Proposing to mark this closed as WONTFIX (due to bug 60290).

Comment 19

[Boris Zbarsky [:bzbarsky]]

Reopening.  Please do NOT wontfix bugs without module owner permission (in this case, see comment 17).  At the very least, this bug is still being used to track bugs on specific instances of appending that can be eliminated.

Comment 20

[Mats Palmgren (inactive)]

This bug appears to be about old style system code that is now gone.