Bug 35362

When running the "Complex Graphics" test from the Peacekeeper suite, upwards of 25% of CPU time is taken parsing CSS rgb() values. Implementing a simple ad-hoc parser (as is done for hex color values) would go a long way to improving performance on this test.

Created attachment 49447 [details] First draft

Re: patch1: This functionality could be moved up into the other flavor of parseColor() on line 278. The layout tests seem ok with it, but I wasn't sure if this is correct.

*** Bug 23110 has been marked as a duplicate of this bug. ***

You should just do rgba() as well.

Comment on attachment 49447 [details] First draft String doesn't guarantee null termination, you should instead pass in a length parameter, then you can do: const UChar* end = *str + length; and the *c pieces become c != end

Created attachment 49597 [details] Patch

Ok, fixed. Also changed to use isCSSWhitespace(), since it doesn't include vertical tab, and isASCIISpace() does.

(In reply to comment #5) > You should just do rgba() as well. Unfortunately, rgba() requires parsing of floats (for the alpha), which is a bit trickier than ints. I'll give it a shot but I might end up doing another patch for it if that's ok.

Comment on attachment 49597 [details] Patch I would be interested in what you get for perf numbers if you replace isCSSWhitespace with a simplified ' ' and '\t' only check -- on the basis we are wanting to get the maximum perf out of the normal case. Also my reading of this seems to indicate that this will accept rgb(1,1,1)<random text>

(In reply to comment #9) > (In reply to comment #5) > > You should just do rgba() as well. > > Unfortunately, rgba() requires parsing of floats (for the alpha), which is a > bit trickier than ints. I'll give it a shot but I might end up doing another > patch for it if that's ok. I think it's important to understand you don't need to parse all valid inputs, you only need the fast path to handle the common cases, eg. 0.[0-9]{1,5} say

(In reply to comment #10) > I would be interested in what you get for perf numbers if you replace > isCSSWhitespace with a simplified ' ' and '\t' only check -- on the basis we > are wanting to get the maximum perf out of the normal case. I would think space only. I expect that very few cases have tabs.

Created attachment 49602 [details] Patch

(In reply to comment #10) > (From update of attachment 49597 [details]) > I would be interested in what you get for perf numbers if you replace > isCSSWhitespace with a simplified ' ' and '\t' only check -- on the basis we > are wanting to get the maximum perf out of the normal case. Ok, I gave it a try. The difference was negligible: +1.75% one run, -1.8% the next, so within the variance of this particular test, which is about +/-2%. (Note that this test is +52.7% faster overall with my fix). > Also my reading of this seems to indicate that this will accept > rgb(1,1,1)<random text> Fixed. Note that I'm not accepting trailing whitespace either, which I figure is the common case (no whitespace after the paren).

Comment on attachment 49602 [details] Patch > +static inline bool parseInt(const UChar** str, const UChar* end, UChar terminator, int* value) For in/out arguments we normally use references rather than pointers. Thus "str" and "value" would typically be references. > + const UChar *c = *str; > + int v = 0; The "*" here is in the wrong place, should be next to the UChar. We normally use words to name local variables, not letters. > + const UChar* c = name.characters() + 4; We normally use words to name local variables, not letters. > + int r = 0, g = 0, b = 0; We normally define variables on separate lines rather than all on one line. There's no need to initialize these variables. I suggest not doing so. I'm going to say r=me, despite these concerns since they are all stylistic issues.

(In reply to comment #15) > (From update of attachment 49602 [details]) > > +static inline bool parseInt(const UChar** str, const UChar* end, UChar terminator, int* value) > > For in/out arguments we normally use references rather than pointers. Thus > "str" and "value" would typically be references. > > > + const UChar *c = *str; > > + int v = 0; > > The "*" here is in the wrong place, should be next to the UChar. > > We normally use words to name local variables, not letters. > > > + const UChar* c = name.characters() + 4; > > We normally use words to name local variables, not letters. > > > + int r = 0, g = 0, b = 0; > > We normally define variables on separate lines rather than all on one line. > > There's no need to initialize these variables. I suggest not doing so. > > I'm going to say r=me, despite these concerns since they are all stylistic > issues. Fixed. (I wasn't sure if you meant "this is ok, but fix these before landing", "fix and re-upload" or "can land as is", so I fixed and re-uploaded just to be safe).

Created attachment 49612 [details] Patch

Committed r55308: <http://trac.webkit.org/changeset/55308>