80106 - [charset]Improve @charset parsing in CSSLoaderImpl::SetCharset

Status: RESOLVED FIXED

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

Description

[nhottanscp]

Separated from bug 72658.

http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsCSSLoader.cpp#1607

Quoted from bug 72658.

> ------- Additional Comments From David Baron 2001-05-09 13:14 -------
>
> I think it would be nice if we didn't make a copy of all of the data in (the
> beginning of?) the stylesheet (at least I'm guessing that's what
> |aStyleSheetData| is) after the @charset and then call
> |nsString::StripWhitespace| on it, just to get an nsAutoString that contains
> what's between the @charset and the ; following.  This could be done easily by
> switching the order of the |aStyleSheetData.Find| (which is now |str.Find|) and
> the |aStyleSheetData.Mid| and using the third parameter to |nsString::Find| or
> (a bit less easily considering the current code) using an nsPromiseSubstring (to
> avoid any copying at all).
>
> (I'm also a bit concerned that we're not parsing the @charset according to CSS's
> parsing rules.)

Comment 1

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

It would also be good not to do an extra one-byte -> two-byte conversion in
SheetLoadData::OnStreamComplete.  That conversion might make somee sense if
AssignWithConversion did a UTF8->UCS2 conversion, but it doesn't.  (It does
ISO-8859-1 to UCS2 conversion.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Taking this....

The attached patch does the following:

1)  Uses dependent strings to avoid having to make a string copy for the
    @charset parsing
2)  Changes the parsing somewhat to be more compliant with the CSS2 grammar (the
    two LookForCharsetRule functions).
3)  Eliminates the string copy that we used to do to get a unichar stream.  We
    now just create a unicode conversion stream and convert as we go. (should
    save some memory allocation if nothing else).
4)  Take all the charset handling out of the CSSLoader proper and move it into
    static functions called directly by the LoadData, since that's what handles
    the decoding anyway.

I profiled this and I _think_ I see a bit of a speed increase, but my data is
not too trustworthy... my jprof profiles of startup had 200-250 hits total, with
0-3 of them in the @charset parsing stuff.  With my changes that dropped to 0-1
instead of 0-3, but I'd love it if someone with a slower machine or a better
profiling setup would profile this one...

Also, any suggestions on 4-byte encodings and what to do with them are very much
welcome.  :)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 55132 [details] [diff] [review]
Proposed patch

um... that would be the wrong patch file. :)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that actually compiles

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 55140 [details] [diff] [review]
Patch that actually compiles

Attaching less ambitious page

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that fixes _this_ bug.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I've focused the patch more on the actual @charset rule parsing.  The other
things it did will be resolved in bug 106843, which this no longer depends on.

Reviews?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh.  I'll remove the #ifdef DEBUG_bzbarsky stuff before checking this in, by the
way.  Just so people know as they review.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 55408 [details] [diff] [review]
Patch that fixes _this_ bug.

comparators have changed headers

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v.3

Comment 12

[Pierre Saslawsky]

I won't be able to spend much time on reviewing this cleanup but at first sight:
- The CSSLoader should not deal with encoding issues the way it is done in 
GetCharsetFromData().
- The two LookForCharsetRule functions are quite redundant.
- We neeed more data about the benefits of this patch in terms of performance and 
memory footprint.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> The CSSLoader should not deal with encoding issues the way it is done in
> GetCharsetFromData().

Well... We have to know whether it's a single-byte or multi-byte encoding to
usefully look for _anything_ in the raw data.  Our current behavior breaks for
multi-byte encodings as far as I can tell.  We could just stay compatible with
that, but that seems wrong.  nhotta, is there an intl API for doing what we want
done there?

I'll work on the other two points.

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK.  I've done some measurements and had rjesup profile this as well.  As far as
I can tell, the time spent in this function is basically nearly zero.  I cannot
get a measurable time spent in it in current builds, so any changes that I make
can't really be an improvement.  Pushing this out till we have some data to show
that this is actually a problem.

Comment 15

[Alec Flett]

so I posted about part of this bug on n.p.m.performance - the bit about passing
around the stream loader instead of multiply copying the data being read in from
necko. Here's an excerpt:
---
In a startup of about:blank, we allocate and release 195k of these decoded style
sheets in 29 allocations. It's the #4 weighted allocation of allocations lasting
less than 4 seconds.

Now, maybe it's just me, but we couldn't we use the i18n nsConverterInputStream,
which has a fixed-size buffer where it does the conversion... we would take the
raw char* string that comes from SheetLoadData::OnStreamComplete, and get a
nsIUnicharInputStream, and pass THAT to the parser.. that would avoid the
overhead of allocating memory for the entire decoded style sheet.
--

so for bloat numbers, I would see a drop of probably about 200k on startup. 

I don't know about the charset parsing stuff though - should we split the above
into a seperate bug?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Not getting to this anytime soon... 

Comment 17

[Boris Zbarsky [:bzbarsky]]

This got done as a side-effect of bug 119321.