Closed Bug 42953 Opened 23 years ago Closed 5 years ago

too much string appending in CSS Parser


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






(Reporter: dbaron, Unassigned)



(Keywords: memory-footprint, perf)


(1 file)

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)
Keywords: perf
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?
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...
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 :-).
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.
Assignee: dbaron → attinasi
Keywords: footprint
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.
Target Milestone: --- → Future
Perhaps the best solution here is to generate the CSSText properties dynamically 

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

Pulling in from Future: performance work is back on the radar...
Target Milestone: Future → mozilla0.9
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.
Moving out to Mozilla 1.0 milestone - small benefit in terms of performance and 
Target Milestone: mozilla0.9 → mozilla1.0
Moving to correct QA Contact- here ya go Ian!
QA Contact: ckritzer → ian
code level bug -> dbaron
QA Contact: ian → dbaron
Daniel Glazman fixed part of this problem as bug 70995.
Reassigning to Pierre. 

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

Assignee: attinasi → pierre
Target Milestone: mozilla1.0 → Future
First we must check whether there is still something to improve.  Boris, do you 
want to look into it?
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
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

Anything else that misuses strings that I'm missing?
This profile still shows a good bit of time spent doing string appending.
Blocks: 106356
Depends on: 311566
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).
Closed: 18 years ago
Resolution: --- → WONTFIX
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.
Resolution: WONTFIX → ---
Assignee: dbaron → nobody
QA Contact: dbaron → style-system
Depends on: 399349
Priority: P3 → P5
This bug appears to be about old style system code that is now gone.
Closed: 18 years ago5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.