Bug 46066 – SVG: Make SVGLength's stringToLengthType() stricter and faster

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Andreas Kling]

stringToLengthType() in SVGLength.cpp is currently testing String::endsWith("foo") which causes temporary WTF::Strings to be created.

We can easily hand-compare the characters to speed up this function.

[Andreas Kling]

Created attachment 68042 [details]
Proposed patch

This patch yields a 10.5% speedup on http://data.xeoh.net/svg.benchmark/

[Antonio Gomes]

(In reply to comment #1)
> Created an attachment (id=68042) [details]
> Proposed patch
> 
> This patch yields a 10.5% speedup on http://data.xeoh.net/svg.benchmark/

L_Great_TM :)

[Dirk Schulze]

Comment on attachment 68042 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68042&action=prettypatch

> WebCore/svg/SVGLength.cpp:97
> +    if (characters[length - 1] == '%')

Can you put UChar b = characters[length - 1]; right before this check? The names should indicate the content. Can you use 'lastChar' for b or simply 'end'? a also needs a better name. Just a bit strange that we give back number, if the string doesn't end with a length type that we know.

> WebCore/svg/SVGLength.cpp:104
> +    UChar a = characters[length - 2];
> +    UChar b = characters[length - 1];
Should be const.

I was looking at setValueAsString, the caller of this function. And it looks bogus to me, to just check the last or last two chars. Following example would parse on WebKit, while it doesn't on other Viewers:
<rect width="20    mm" height="20*SomethinStupid*px"/>

Can't you just use 'ptr' and 'end' of setValueAsString? 'ptr' is the first char after the parsed number of the string. We don't need String at all on lengthTypeToString. This change will need new layouttests.

[Andreas Kling]

Created attachment 68733 [details]
Proposed patch v2

Strictified SVGLength to only accept unit types immediately following the numeric value, and with no whitespace after.
This behavior matches Firefox, but not IE9 or Opera.
IE9 accepts whitespace on both sides of the unit type.
Opera doesn't accept whitespace between the numeric value and the unit type, but accepts it after the unit type.

[Dirk Schulze]

Comment on attachment 68733 [details]
Proposed patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=68733&action=review

The code is not realy faster with the mentioned changes, but looks more like the rest of the parsing code in SVGParserUttilities.cpp. And it may make it easier to change the code to ignore whitespaces at the end without parsing error later (the default behavior on Opera, but not specified yet) by just adding two more lines. The test looks great! Great patch!

> WebCore/svg/SVGLength.cpp:89
> +inline SVGLengthType stringToLengthType(const UChar* characters, const UChar* end)

const UChar*& ptr

> WebCore/svg/SVGLength.cpp:94
> +    unsigned length = end - characters;
> +
> +    if (!length)
> +        return LengthTypeNumber;

if (ptr == end)
 return LengthTypeNumber;

> WebCore/svg/SVGLength.cpp:105
> +    if (length != 1 && length != 2)
> +        return LengthTypeUnknown;
> +
> +    const UChar lastChar = characters[length - 1];
> +
> +    if (length == 1) {
> +        if (lastChar == '%')
> +            return LengthTypePercentage;
> +        return LengthTypeUnknown;
> +    }

const UChar firstChar = *ptr;
++ptr;
if (firstChar == '%') {
  if (ptr == end)
    return LengthTypePercentage;
  return LengthTypeUnkown;
}

const UChar secondChar = *ptr;
if (++ptr != end)
  return LengthTypeUnkown;

[Andreas Kling]

Created attachment 68761 [details]
Proposed patch v3

Updated with Dirk's suggestions.

[Dirk Schulze]

Comment on attachment 68761 [details]
Proposed patch v3

LGTM. r=me.

[WebKit Commit Bot]

Comment on attachment 68761 [details]
Proposed patch v3

Clearing flags on attachment: 68761

Committed r68307: <http://trac.webkit.org/changeset/68307>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.