CSS parsing is 5.5% of startup time
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
Performance Impact | medium |
People
(Reporter: dbaron, Unassigned)
References
(Depends on 2 open bugs, Blocks 2 open bugs)
Details
(Keywords: arch, perf, perf:startup, Whiteboard: [ts][meta])
Attachments
(1 file)
243.19 KB,
text/html
|
Details |
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.
Reporter | ||
Comment 1•23 years ago
|
||
Comment 2•23 years ago
|
||
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•23 years ago
|
||
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...).
Reporter | ||
Updated•23 years ago
|
Comment 4•23 years ago
|
||
Perrie, why is this future ? I can work on this. 5.5% is too big to pass.
Comment 5•23 years ago
|
||
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.
Updated•23 years ago
|
Comment 6•23 years ago
|
||
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•23 years ago
|
||
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.
Reporter | ||
Comment 8•22 years ago
|
||
Assigning pierre's remaining Style System-related bugs to myself.
Comment 9•22 years ago
|
||
Does fastload affect this?
Reporter | ||
Comment 10•22 years ago
|
||
No, since we don't fastload CSS yet, except that it's gradually becoming a larger percentage.
Reporter | ||
Updated•22 years ago
|
Updated•20 years ago
|
Reporter | ||
Updated•17 years ago
|
Comment 11•16 years ago
|
||
With all the CSS work, is there any improvement on this bug?
Updated•15 years ago
|
Comment 12•15 years ago
|
||
Taras, do you have contemporary numbers on what percentage of startup is spent parsing CSS?
Updated•15 years ago
|
Updated•15 years ago
|
Comment 14•15 years ago
|
||
I'm willing to take ownership of CSS parser performance generally.
Updated•15 years ago
|
Updated•15 years ago
|
Updated•14 years ago
|
Updated•13 years ago
|
Updated•13 years ago
|
Reporter | ||
Updated•13 years ago
|
Comment 15•2 years ago
|
||
Is this any better since 102.x?
Updated•2 years ago
|
Comment 16•1 year ago
|
||
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•1 year ago
|
||
This requires up-to-date profiling.
Comment 18•1 year ago
|
||
(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•1 year ago
|
||
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
Description
•