Summary: | CSS: Add fast-path for rgba() color parsing | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | darin, eric, hausmann, oliver | ||||||||||||
Priority: | P2 | Keywords: | Performance | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Andreas Kling
2010-07-26 04:45:47 PDT
Created attachment 62561 [details]
Proposed patch
Attachment 62561 [details] did not build on mac: Build output: http://queues.webkit.org/results/3601387 Created attachment 62689 [details]
Proposed patch v2
Same patch with fixes for the 2 warnings from the EWS.
Created attachment 62690 [details]
Proposed patch v2
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. 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.
Attachment 63183 [details] did not build on mac: Build output: http://queues.webkit.org/results/3619464 (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 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 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?
Created attachment 63446 [details]
Proposed patch v4
Updated to support ".0" through ".9" as well. Fleshed out the layout test a bit.
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?
(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.) Here's the relevant spec bit about alpha value clamping: http://www.w3.org/TR/css3-color/#alphavaluedt @Darin: Ping. :-) 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 (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. Committed r65475: <http://trac.webkit.org/changeset/65475> Revision r65475 cherry-picked into qtwebkit-2.1 with commit aa4d1183733f4669dcf3ef89b48ae01b2fbc3fa0 |