91242 - CSS parsing is 5.5% of startup time

Status: RESOLVED WORKSFORME

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

Description

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

CSS parsing, according to a jprof profile I just took on Linux, is about 5.5% of
startup time.  The time spent can be divided into two parts (although actually
splitting the profile between them would be hard):  string manipulation and data
structure construction.

There are two approaches I can think of to the problems that we spend a lot of
time mucking about with strings:

 1) Improve the string use within the CSS parser by using the new string code
and doing less copying.  This could probably result in a significant improvement
in the string manipulation part.

 2) Store the CSS stylesheet data structures in the fastload file (see bug
68045).  This would mean reconstructing the data structures out of the fastload
file.  I'm not sure what the performance of the fastload parsing is -- unlike JS
script compilation, the processing done by the CSS parser is mostly very simple,
so I'm not sure how big the win of replacing one parser with another would be,
although there are certainly some things it would improve.

I need to look more at the data structure construction bit.  I'm not sure how
much there really is beyond spending time in malloc.

I'll attach a profile of the CSS parsing segment of startup.

Comment 1

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

Attachment: profile of the CSS parsing part of startup

Comment 2

[Pierre Saslawsky]

glazman worked on reducing string manipulation a few months ago, he may have more 
info on this. I think that we still do 2 or 3 copies of the data.

Marking future for now but will re-prioritize when David comes with more stats.

Comment 3

[Pierre Saslawsky]

On a related matter, this is what Marc Attinasi wrote about a year ago in
news://news.mozilla.org/3905EE19.361F497B%40netscape.com

    ----

I was looking into the SheetLoadData and CSSLoaderImpl string use and I
found an interesting issue. Clearly, the two are using nsStrings to the
exact same extent...

The CSSLoader is duplicating the data coming in from the net (resulting
in triplicate data, actually). The CSSLoader uses the SheetLoaderData to
load the data. The SheetLoaderData gets data from the netwerk and then
it makes a copy of it in an nsString, decoded in the correct charset.
The decoded string is then passed from the SheetLoadData to the
CSSLoader which makes another nsString copy of it to pass to
StringUnicharInputStream. It seems that we could (should?) eliminate the
second copy of the string data in the CSSLoaderImpl::DidLoadStyle
method, however the current implementation of the
StringUnicharInputStream takes ownership of the string so we may have to
change that (we could change the way the LoadData passes the string to
the Loader so that it passes ownership and allows the
StringUnicharInputStream to delete the original too, which seems simpler
actually). This should be a huge benefit, I'll look into the change. It
could cut in half the string allocations related to loading the style
sheets.

To summarize, this is the current situation:

  Netwerk --- (data as char*) -->
      SheetLoadData --- (copies data to stack-based nsString) -->
          CSSLoader --- (makes another copy of the nsString passed in
from SheetLoadData) -->
              StringUnicharInputStream --- (takes ownership of nsString
and deletes when done) --|

Possible optimization (eliminates one nsString copy of the data):

  Netwerk --- (data as char*) -->
      SheetLoadData --- (copies data to heap-allocated nsString) -->
          CSSLoader --- (takes ownership of nsString passed in from
SheetLoadData) -->
              StringUnicharInputStream --- (takes ownership of nsString
and deletes when done) --|

OK, I tried the change and it works correctly, as you would expect. This
should result in 572K less bloat at load. This one was pretty easy to
fix in the CSSLoader, but in general it is a bigger problem to control
ownership effectively and at the same time avoid duplicating data - it
seems the more we avid duplicating data the murkier the ownership issues
become. I'll check changes to CSSLoader and SheetLoadData  in as soon as
I have them reviewed and the tree is open.

To answer a question posed below, all of my style memory stats are from
Viewer, not from Mozilla. Also, my data shows memory footprint at
seady-state, not transient data like the BloatStats show (from what
little I understand about the bloat stats...).

Comment 4

[Suresh Duddi (gone)]

Perrie, why is this future ? I can work on this. 5.5% is too big to pass.

Comment 5

[Pierre Saslawsky]

It is related to charset and string conversion issues.  Boris is working on it 
right now.  I propose we wait until he completes what he is doing and take a new 
set of metrics.  It may not solve everything but it is a necessary first step.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ok.  One thought I have so far....  As Marc pointed out, we currently do:

Netwerk --- (data as char*) -->
SheetLoadData --- (copies data to heap-allocated nsString) -->
CSSLoader --- (takes ownership of nsString passed in from SheetLoadData) -->
StringUnicharInputStream --- (takes ownership of nsString and deletes when done)
--|

A possible alternative is:

Netwerk --- (data as char*) -->
SheetLoadData -- (wraps the char* in an nsIInputStream, then wraps that in a
                 converter stream (see
   http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsIUnicharInputStream.h#73))
-->  CSSLoader -- (just passes the converter stream on to the CSS parser) --|

This seems like it would somewhat simplify ownership issues and avoid creating
an extra copy of the data, since it looks like ConverterInputStream does
conversion as needed when data is read.  I don't know how it would affect
performance, to be truthful, but the CSS scanner seems to read data in
decently-sized chunks from the stream so there should not be much more overhead
there.

One immediate problem with this is that currently if we fail to convert we drop
the entire sheet in SheetLoadData::OnStreamComplete.  With conversion as-we-go,
we would be partway through the sheet and encounter a conversion error. 
Currently it looks like this will be treated the same way as a premature end of
file...  In particular, the parser will create a sheet with all the data up to
the point where the conversion error occured and return it.  This could
certainly be changed if we wish by just looking at the error code in the
nsIParser interface functions that the outside world calls.

Thoughts?

Comment 7

[Pierre Saslawsky]

After the work Boris did, I kept this bug opened mainly as a reminder to check 
the improvements after reducing the number of memory allocations in the style 
system (bug 106356).  Updated dependencies (106356 is indirectly dependent on 
7251 already) and pushed to future.

Comment 8

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

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

Comment 9

[Hixie (not reading bugmail)]

Does fastload affect this?

Comment 10

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

No, since we don't fastload CSS yet, except that it's gradually becoming a
larger percentage.

Comment 11

[Worcester12345]

With all the CSS work, is there any improvement on this bug?

Comment 12

[Dietrich Ayala (:dietrich)]

Taras, do you have contemporary numbers on what percentage of startup is spent parsing CSS?

Comment 13

[Dietrich Ayala (:dietrich)]

meta bug, not P1.

Comment 14

[Zack Weinberg (:zwol)]

I'm willing to take ownership of CSS parser performance generally.

Comment 15

[Arthur K. (he/him)]

Is this any better since 102.x?

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 13 votes.
:emilio, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 17

[Emilio Cobos Álvarez [:emilio]]

This requires up-to-date profiling.

Comment 18

[Arthur K. (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

This requires up-to-date profiling.

Perhaps. But after 22 years of this bug not being closed and 11 years since the last comment (besides mine), might be ok to take a leap of faith and say this is probably no longer an issue.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I ran this through the performance impact calculator, perhaps generously characterising it as "noticable startup delay" but I agree we should gather fresh data. If indeed we have a ~5% startup delay on all platforms due to CSS parsing then this could be considered a medium impact. Otherwise, we can retriage when we have up to date profiling.

The Performance Impact Calculator has determined this bug's performance impact to be medium. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.

Platforms: [x] Windows [x] macOS [x] Linux
Impact on browser: Causes noticeable startup delay

Comment 20

[Markus Stange [:mstange]]

Here's a startup profile from macOS: https://share.firefox.dev/4f7Wy1s

When selecting the root node in the call tree and looking at the category breakdown in the sidebar, I see 3 samples in CSS parsing, corresponding to 0.3% of the total sample count.

So I can confirm that this is no longer a problem.