Bug 42965 - CSS: Add fast-path for rgba() color parsing
Summary: CSS: Add fast-path for rgba() color parsing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2010-07-26 04:45 PDT by Andreas Kling
Modified: 2010-08-24 05:55 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (3.37 KB, patch)
2010-07-26 04:47 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch v2 (4.07 KB, patch)
2010-07-27 07:58 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch v2 (3.36 KB, patch)
2010-07-27 07:59 PDT, Andreas Kling
darin: review-
Details | Formatted Diff | Diff
Proposed patch v3 (404.37 KB, patch)
2010-08-02 00:59 PDT, Andreas Kling
eric: review-
Details | Formatted Diff | Diff
Proposed patch v4 (38.87 KB, patch)
2010-08-04 06:23 PDT, Andreas Kling
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-07-26 04:45:47 PDT
Currently #HHEEXX, #HEX, rgb() and named colors are supported by the CSSParser::parseColor() fast-path.

We should add support for rgba() colors as well.

Example of a page which spends way too much time parsing rgba() colors:
http://www.mrspeaker.net/dev/parcycle/
Comment 1 Andreas Kling 2010-07-26 04:47:59 PDT
Created attachment 62561 [details]
Proposed patch
Comment 2 Eric Seidel 2010-07-26 04:57:09 PDT
Attachment 62561 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3601387
Comment 3 Andreas Kling 2010-07-27 07:58:52 PDT
Created attachment 62689 [details]
Proposed patch v2

Same patch with fixes for the 2 warnings from the EWS.
Comment 4 Andreas Kling 2010-07-27 07:59:37 PDT
Created attachment 62690 [details]
Proposed patch v2
Comment 5 Darin Adler 2010-07-27 10:22:46 PDT
Comment on attachment 62690 [details]
Proposed patch v2

> +static inline bool parseAlphaDouble(const UChar*& string, const UChar* end, UChar terminator, double& value)
> +{
> +    int length = end - string;
> +    if (!length)
> +        return false;
> +
> +    Vector<char, 256> bytes(length + 1);
> +    for (int i = 0; i < length; ++i) {
> +        if (!isASCIIDigit(string[i]) && string[i] != '.' && string[i] != ')')
> +            return false;
> +        bytes[i] = string[i];
> +    }
> +    bytes[length] = '\0';
> +    char* foundTerminator;
> +    value = WTF::strtod(bytes.data(), &foundTerminator);
> +    value = max(0.0, min(value, 1.0));
> +    string += (foundTerminator - bytes.data()) + 1;
> +    return *foundTerminator == terminator;
> +}

For the typical alpha values like 0, 1, and the tenths and simple powers of too we can do a *lot* faster than calling the general purpose strtod function every time, perhaps even avoiding floating point math altogether.

> +        rgb = makeRGBA(red, green, blue, static_cast<int>(lroundf(255.0f * static_cast<float>(alpha))));

I don’t think it’s correct to multiply the alpha value by 255. At other call sites you'll see code more like this:

    nextafter(256, 0) * alpha

We need some test cases that cover the edge cases, the boundaries between alpha of 254 and 255 for example, and alpha of 0 and 1. The relevant functions to look at are CSSParser::parseColorParameters and the various functions in the Color.cpp file. There may be some cases where multiplying by 255 is right. Maybe we can also do code sharing.

If the test cases prove that you are not changing behavior, then I suppose you could put this patch up for review again.

review- because of the 255/256 thing.
Comment 6 Andreas Kling 2010-08-02 00:59:30 PDT
Created attachment 63183 [details]
Proposed patch v3

Updated patch.
* Use nextafter(256.0, 0.0)
* Avoid strtod() for "0", "1", and "0.x"
* Added a test to verify that we're not inconsistent with the slow-path.
Comment 7 Eric Seidel 2010-08-02 01:03:59 PDT
Attachment 63183 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3619464
Comment 8 Andreas Kling 2010-08-02 04:22:26 PDT
(In reply to comment #7)
> Attachment 63183 [details] did not build on mac:
> Build output: http://queues.webkit.org/results/3619464

/Users/eseidel/Projects/MacEWS/WebCore/css/CSSParser.cpp: In static member function 'static bool WebCore::CSSParser::parseColor(const WebCore::String&, WebCore::RGBA32&, bool)':
/Users/eseidel/Projects/MacEWS/WebCore/css/CSSParser.cpp:3811: warning: 'alpha' may be used uninitialized in this function

False alarm AFAICS. I can jam a "value = 0;" at the start of parseAlphaValue() to silence it if desired.
Comment 9 Eric Seidel 2010-08-02 07:30:53 PDT
Comment on attachment 63183 [details]
Proposed patch v3

False alarm or not, it breaks Mac, which is a No-Go.

It's definitely possible to return out of parseAlphaValue without setting "value" which may be what's confusing the compiler?

Then again the compiler could be right and we could be reading something wrong here.
Comment 10 Darin Adler 2010-08-02 10:55:09 PDT
Comment on attachment 63183 [details]
Proposed patch v3

Initializing alpha to 0 is probably the simplest way to get rid of that warning.

Are strings like ".1" or ".9" valid for alpha, as opposed to "0.1" and "0.9"? Do we have test cases that cover that?
Comment 11 Andreas Kling 2010-08-04 06:23:11 PDT
Created attachment 63446 [details]
Proposed patch v4

Updated to support ".0" through ".9" as well. Fleshed out the layout test a bit.
Comment 12 Darin Adler 2010-08-04 14:21:14 PDT
Comment on attachment 63446 [details]
Proposed patch v4

What is an alpha value such as "2" supposed to do? Does it still do what it used to do before?
Comment 13 Andreas Kling 2010-08-05 04:17:42 PDT
(In reply to comment #12)
> (From update of attachment 63446 [details])
> What is an alpha value such as "2" supposed to do? Does it still do what it used to do before?

Alpha values should be clamped between 0.0 and 1.0, so "2" will result in 1.0 (via the slow-path.)
Comment 14 Andreas Kling 2010-08-16 15:44:24 PDT
Here's the relevant spec bit about alpha value clamping:
http://www.w3.org/TR/css3-color/#alphavaluedt

@Darin: Ping. :-)
Comment 15 Darin Adler 2010-08-16 16:30:03 PDT
Comment on attachment 63446 [details]
Proposed patch v4

> +    int length = end - string;
> +    if (length < 2)
> +        return false;

So just plain "0" and "1" are not in the fast path?

> +    if ((length == 4 && string[0] == '0' && string[1] == '.' && isASCIIDigit(string[2])
> +        || (length == 3 && string[0] == '.' && isASCIIDigit(string[1])))) {

Might be nice to use a helper function just to make this if statement easier to read.

> +        switch (string[length - 2]) {
> +        case '0': value = 0; break;
> +        case '1': value = 25; break;
> +        case '2': value = 51; break;
> +        case '3': value = 76; break;
> +        case '4': value = 102; break;
> +        case '5': value = 127; break;
> +        case '6': value = 153; break;
> +        case '7': value = 179; break;
> +        case '8': value = 204; break;
> +        case '9': value = 230; break;
> +        }

Seems like a small array would work better here than a switch.

> +    Vector<char, 256> bytes(length + 1);

The number 256 seems a little high here.

r=me
Comment 16 Andreas Kling 2010-08-16 16:33:59 PDT
(In reply to comment #15)
> (From update of attachment 63446 [details])
> > +    int length = end - string;
> > +    if (length < 2)
> > +        return false;
> 
> So just plain "0" and "1" are not in the fast path?

'length' includes the terminating character.

> r=me

Hugs! Will make the requested adjustments before landing.
Comment 17 Andreas Kling 2010-08-16 18:10:21 PDT
Committed r65475: <http://trac.webkit.org/changeset/65475>
Comment 18 Simon Hausmann 2010-08-24 05:45:50 PDT
Revision r65475 cherry-picked into qtwebkit-2.1 with commit aa4d1183733f4669dcf3ef89b48ae01b2fbc3fa0