Bug 46066 - SVG: Make SVGLength's stringToLengthType() stricter and faster
Summary: SVG: Make SVGLength's stringToLengthType() stricter and faster
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Keywords: Performance
Depends on:
Reported: 2010-09-19 17:39 PDT by Andreas Kling
Modified: 2010-09-24 15:57 PDT (History)
4 users (show)

See Also:

Proposed patch (2.36 KB, patch)
2010-09-19 17:43 PDT, Andreas Kling
krit: review-
Details | Formatted Diff | Diff
Proposed patch v2 (8.06 KB, patch)
2010-09-24 12:46 PDT, Andreas Kling
krit: review-
Details | Formatted Diff | Diff
Proposed patch v3 (7.95 KB, patch)
2010-09-24 15:01 PDT, Andreas Kling
no flags 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-09-19 17:39:03 PDT
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.
Comment 1 Andreas Kling 2010-09-19 17:43:47 PDT
Created attachment 68042 [details]
Proposed patch

This patch yields a 10.5% speedup on http://data.xeoh.net/svg.benchmark/
Comment 2 Antonio Gomes 2010-09-19 18:02:52 PDT
(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 :)
Comment 3 Dirk Schulze 2010-09-19 23:55:12 PDT
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.
Comment 4 Andreas Kling 2010-09-24 12:46:29 PDT
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.
Comment 5 Dirk Schulze 2010-09-24 14:24:36 PDT
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;
if (firstChar == '%') {
  if (ptr == end)
    return LengthTypePercentage;
  return LengthTypeUnkown;

const UChar secondChar = *ptr;
if (++ptr != end)
  return LengthTypeUnkown;
Comment 6 Andreas Kling 2010-09-24 15:01:23 PDT
Created attachment 68761 [details]
Proposed patch v3

Updated with Dirk's suggestions.
Comment 7 Dirk Schulze 2010-09-24 15:10:21 PDT
Comment on attachment 68761 [details]
Proposed patch v3

LGTM. r=me.
Comment 8 WebKit Commit Bot 2010-09-24 15:57:33 PDT
Comment on attachment 68761 [details]
Proposed patch v3

Clearing flags on attachment: 68761

Committed r68307: <http://trac.webkit.org/changeset/68307>
Comment 9 WebKit Commit Bot 2010-09-24 15:57:39 PDT
All reviewed patches have been landed.  Closing bug.