Bug 213635 – Convert SVG related parsers over to using StringParsingBuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Convert SVG related parsers over to using StringParsingBuffer

[Sam Weinig]

Created attachment 402846 [details]
Patch

[Sam Weinig]

Created attachment 402867 [details]
Patch

[Sam Weinig]

Created attachment 402906 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 402906 [details]
Patch

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

> Source/WebCore/svg/SVGLengthList.cpp:75
> +String SVGLengthList::valueAsString() const
> +{
> +    StringBuilder builder;
> +
> +    for (const auto& length : m_items) {
> +        if (builder.length())
> +            builder.append(' ');
> +
> +        builder.append(length->value().valueAsString());
> +    }
> +
> +    return builder.toString();
> +}

Since you are moving this function and others similar to it from the header files to cpp files, I would suggest removing all the valueAsString() methods from the SVG list classes and implement:

1. SVGValuePropertyList::valueAsString() 
2. SVGPrimitiveList::valueAsString(). 

The loop in these two functions will the same as this one. The only difference is:

1. SVGValuePropertyList will append item->valueAsString() 
2. SVGPrimitiveList will append SVGPropertyTraits<PropertyType>::toString(item). 

This will remove the duplicate code and will make the cpp files mainly for parsing.

[Darin Adler]

Comment on attachment 402906 [details]
Patch

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

> Source/WebCore/svg/SVGAnimationElement.cpp:81
> +static Optional<Vector<UnitBezier>> parseKeySplines(const StringView& string)

Shouldn't we prefer StringView over const StringView&?

> Source/WebCore/svg/SVGAnimationElement.cpp:84
> +    if (string.isEmpty())
>          return WTF::nullopt;

Is this a valuable optimization? I don’t think it is. Or is it needed for correctness.

> Source/WebCore/svg/SVGFitToViewBox.cpp:88
>  Optional<FloatRect> SVGFitToViewBox::parseViewBox(const StringView& value)

Shouldn't we prefer StringView over const StringView&?

> Source/WebCore/svg/SVGLengthList.h:55
> +    bool parse(const String& value);

Any downside in changing this to take StringView?

> Source/WebCore/svg/SVGLengthValue.cpp:71
> +    auto firstChar = *buffer;
> +    ++buffer;

Why not *buffer++?

> Source/WebCore/svg/SVGLengthValue.cpp:77
> +    auto secondChar = *buffer;
> +    ++buffer;

Ditto.

> Source/WebCore/svg/SVGLengthValue.h:66
> +    static Optional<SVGLengthValue> construct(SVGLengthMode, const StringView&);
> +    static SVGLengthValue construct(SVGLengthMode, const StringView&, SVGParsingError&, SVGLengthNegativeValuesMode = SVGLengthNegativeValuesMode::Allow);

StringView instead of const StringView&?

> Source/WebCore/svg/SVGLengthValue.h:90
> +    ExceptionOr<void> setValueAsString(const StringView&);
> +    ExceptionOr<void> setValueAsString(const StringView&, SVGLengthMode);

StringView instead of const StringView&?

> Source/WebCore/svg/SVGNumberList.cpp:65
> +    StringBuilder builder;
> +
> +    for (const auto& number : m_items) {
> +        if (builder.length())
> +            builder.append(' ');
> +
> +        builder.append(number->value());
> +    }
> +
> +    return builder.toString();

Another idiom we should find a way to improve. I don’t like how we have to write these join loops.

> Source/WebCore/svg/SVGNumberList.h:53
> +    bool parse(const String&);

StringView?

> Source/WebCore/svg/SVGNumberList.h:54
> +    String valueAsString() const override;

final?

> Source/WebCore/svg/SVGParserUtilities.cpp:117
> +            exponent += *buffer - '0';
> +            ++buffer;

*buffer++?

> Source/WebCore/svg/SVGPathSource.h:22
> +#include "FloatPoint.h"

Makes me a tiny bit sad. Not sure what made this happen.

> Source/WebCore/svg/SVGPathStringSource.h:49
> +    template<typename F> decltype(auto) parse(F&&);

I’d prefer a word rather than "F". Is this a function?

> Source/WebCore/svg/SVGPointList.h:53
> +    bool parse(const String& value);

StringView?

> Source/WebCore/svg/SVGPreserveAspectRatioValue.cpp:365
> +        case SVG_PRESERVEASPECTRATIO_NONE:
> +            return "none";
> +        case SVG_PRESERVEASPECTRATIO_XMINYMIN:
> +            return "xMinYMin";
> +        case SVG_PRESERVEASPECTRATIO_XMIDYMIN:
> +            return "xMidYMin";
> +        case SVG_PRESERVEASPECTRATIO_XMAXYMIN:
> +            return "xMaxYMin";
> +        case SVG_PRESERVEASPECTRATIO_XMINYMID:
> +            return "xMinYMid";
> +        case SVG_PRESERVEASPECTRATIO_XMIDYMID:
> +            return "xMidYMid";
> +        case SVG_PRESERVEASPECTRATIO_XMAXYMID:
> +            return "xMaxYMid";
> +        case SVG_PRESERVEASPECTRATIO_XMINYMAX:
> +            return "xMinYMax";
> +        case SVG_PRESERVEASPECTRATIO_XMIDYMAX:
> +            return "xMidYMax";
> +        case SVG_PRESERVEASPECTRATIO_XMAXYMAX:
> +            return "xMaxYMax";
> +        case SVG_PRESERVEASPECTRATIO_UNKNOWN:
> +            return "unknown";

Want some "_s" here I think. Unless the ASCIILiteral optimization is unimportant.

> Source/WebCore/svg/SVGPreserveAspectRatioValue.h:55
> +    SVGPreserveAspectRatioValue(const StringView&);

StringView rather than const StringView&?

> Source/WebCore/svg/SVGPreserveAspectRatioValue.h:67
> +    bool parse(const StringView&);

StringView rather than const StringView&?

> Source/WebCore/svg/SVGPreserveAspectRatioValue.h:78
> +    template<typename CharacterType>
> +    bool parseInternal(StringParsingBuffer<CharacterType>&, bool validate);

On one line?

> Source/WebCore/svg/SVGStringList.h:54
> +    bool parse(const String& data, UChar delimiter);

StringView

> Source/WebCore/svg/SVGTransformList.cpp:59
> +    for (const auto& transform : m_items)

I would have omitted the const here.

> Source/WebCore/svg/SVGTransformList.h:61
> +    void parse(const String&);

StringView

> Source/WebCore/svg/SVGTransformable.cpp:100
> +constexpr int requiredValuesForType[] =  {0, 6, 1, 1, 1, 1, 1};
> +constexpr int optionalValuesForType[] =  {0, 0, 1, 1, 2, 0, 0};

Remove the extra space before "{"?

Not sure we should have removed static. Because constexpr does imply const, but not necessarily internal linkage. Unclear on the considerations.

> Source/WebCore/svg/SVGTransformable.h:37
> +    static Optional<SVGTransformValue::SVGTransformType> parseTransformType(const StringView&);

StringView rather than const StringView&?

[Sam Weinig]

(In reply to Said Abou-Hallawa from comment #4)
> Comment on attachment 402906 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402906&action=review
> 
> > Source/WebCore/svg/SVGLengthList.cpp:75
> > +String SVGLengthList::valueAsString() const
> > +{
> > +    StringBuilder builder;
> > +
> > +    for (const auto& length : m_items) {
> > +        if (builder.length())
> > +            builder.append(' ');
> > +
> > +        builder.append(length->value().valueAsString());
> > +    }
> > +
> > +    return builder.toString();
> > +}
> 
> Since you are moving this function and others similar to it from the header
> files to cpp files, I would suggest removing all the valueAsString() methods
> from the SVG list classes and implement:
> 
> 1. SVGValuePropertyList::valueAsString() 
> 2. SVGPrimitiveList::valueAsString(). 
> 
> The loop in these two functions will the same as this one. The only
> difference is:
> 
> 1. SVGValuePropertyList will append item->valueAsString() 
> 2. SVGPrimitiveList will append
> SVGPropertyTraits<PropertyType>::toString(item). 
> 
> This will remove the duplicate code and will make the cpp files mainly for
> parsing.

I am not really convinced this is worth it. Doing this for all the classes would cause a pessimization of SVGNumberList and SVGPointList due to unnecessary extra allocations of Strings. 

There might be ways to avoid this, but I am not sure it is worth the complexity.

[Sam Weinig]

Created attachment 402936 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #5)
> Comment on attachment 402906 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402906&action=review
> 
> > Source/WebCore/svg/SVGAnimationElement.cpp:81
> > +static Optional<Vector<UnitBezier>> parseKeySplines(const StringView& string)
> 
> Shouldn't we prefer StringView over const StringView&?

Fixed.

> 
> > Source/WebCore/svg/SVGAnimationElement.cpp:84
> > +    if (string.isEmpty())
> >          return WTF::nullopt;
> 
> Is this a valuable optimization? I don’t think it is. Or is it needed for
> correctness.

It's needed for correctness (or at least, to preserve the current behavior).

> 
> > Source/WebCore/svg/SVGFitToViewBox.cpp:88
> >  Optional<FloatRect> SVGFitToViewBox::parseViewBox(const StringView& value)
> 
> Shouldn't we prefer StringView over const StringView&?

Fixed,

> 
> > Source/WebCore/svg/SVGLengthList.h:55
> > +    bool parse(const String& value);
> 
> Any downside in changing this to take StringView?

Fixed.

> 
> > Source/WebCore/svg/SVGLengthValue.cpp:71
> > +    auto firstChar = *buffer;
> > +    ++buffer;
> 
> Why not *buffer++?

Fixed.

> 
> > Source/WebCore/svg/SVGLengthValue.cpp:77
> > +    auto secondChar = *buffer;
> > +    ++buffer;
> 
> Ditto.

Fixed.

> 
> > Source/WebCore/svg/SVGLengthValue.h:66
> > +    static Optional<SVGLengthValue> construct(SVGLengthMode, const StringView&);
> > +    static SVGLengthValue construct(SVGLengthMode, const StringView&, SVGParsingError&, SVGLengthNegativeValuesMode = SVGLengthNegativeValuesMode::Allow);
> 
> StringView instead of const StringView&?

Fixed.

> 
> > Source/WebCore/svg/SVGLengthValue.h:90
> > +    ExceptionOr<void> setValueAsString(const StringView&);
> > +    ExceptionOr<void> setValueAsString(const StringView&, SVGLengthMode);
> 
> StringView instead of const StringView&?

Fixed.

> 
> > Source/WebCore/svg/SVGNumberList.cpp:65
> > +    StringBuilder builder;
> > +
> > +    for (const auto& number : m_items) {
> > +        if (builder.length())
> > +            builder.append(' ');
> > +
> > +        builder.append(number->value());
> > +    }
> > +
> > +    return builder.toString();
> 
> Another idiom we should find a way to improve. I don’t like how we have to
> write these join loops.

Yeah :(. This has been on my mind since I did the variadic append work a while back. I would like to solve it by generalizing StringTypeAdapter to be able to work in a lazier manner in some cases. Currently, StringTypeAdapter really wants to know the total length and is8Bit state upfront, or have you compute them potentially multiple times. A naive solution would end up looking something like this:


builder.append(Join(m_items, ' ', [](auto itemToTransform) { itemToTransform->value() }));

for:

template<typename List, typename Delimiter, typename ListTransformer>
struct Join {
    List& list;
    Delimiter delimiter;
    ListTransformer& listTransformer;
};


template<typename List, typename Delimiter, typename ListTransformer> class StringTypeAdapter<Join<List, Delimiter, ListTransformer>> {
public:
    StringTypeAdapter(const Join<List, Delimiter, ListTransformer>& join)
        : m_join { join }
        , m_delimiterAdapter { m_join.delimiter }
    {
        unsigned numberOfItems = std::size(m_join.list);
        unsigned numberOfDelimiters = numberOfItems > 1 ? numberOfItems - 1 : 0;
        
        m_length = numberOfDelimiters * m_delimiterAdapter.length();
        m_is8Bit = m_delimiterAdapter.is8Bit();

        for (auto& item : m_join.list) {
            auto transformedItem = m_join.listTransformer(item);
            StringTypeAdapter<decltype(transformedItem)> adaptor { transformedItem };
            m_length += adaptor.length();
            if (m_is8Bit && !adaptor.is8Bit())
                m_is8Bit = false;
        }
    }

    unsigned length() const { return m_length; }
    bool is8Bit() const { return m_is8Bit; }
    template<typename CharacterType> void writeTo(CharacterType* destination) const
    {
        bool firstIteration = true;
        for (auto& item : m_join.list) {
            if (!firstIteration) {
                m_delimiterAdapter.writeTo(destination);
                destination += m_delimiterAdapter.length();
            }

            auto transformedItem = m_join.listTransformer(item);
            StringTypeAdapter<decltype(transformedItem)> adaptor { transformedItem };
            m_delimiterAdapter.writeTo(destination);
            destination += adaptor.length();
        }
    }

private:
    const Join<List, Delimiter, ListTransformer>& m_join;
    StringTypeAdapter<Delimiter> m_delimiterAdapter;
    unsigned m_length;
    bool m_is8Bit;
};


(I could also see a version where the adaptor stored a Vector<StringTypeAdapter<decltype(m_join.listTransformer(m_join.list[0]))>>>)

Which leads to the question whether the avoidance of resizes of the StringBuilder storage is worth iterating the loop multiple times.

Other option would be to just add a StringBuilder::appendByJoining(Container&, Delimiter&, Transformer&), Maybe that would be a good improvement even if not perfect.


> 
> > Source/WebCore/svg/SVGNumberList.h:53
> > +    bool parse(const String&);
> 
> StringView?


Fixed.

> 
> > Source/WebCore/svg/SVGNumberList.h:54
> > +    String valueAsString() const override;
> 
> final?

Made the class final, since it wasn't but should be. Is our style to use final on functions as well as the class? I can't recall.

> 
> > Source/WebCore/svg/SVGParserUtilities.cpp:117
> > +            exponent += *buffer - '0';
> > +            ++buffer;
> 
> *buffer++?

Fixed,

> 
> > Source/WebCore/svg/SVGPathSource.h:22
> > +#include "FloatPoint.h"
> 
> Makes me a tiny bit sad. Not sure what made this happen.

Removed a different #include "FloatPoint.h". The header has FloatPoint member variables, so it's hard to avoid.

> 
> > Source/WebCore/svg/SVGPathStringSource.h:49
> > +    template<typename F> decltype(auto) parse(F&&);
> 
> I’d prefer a word rather than "F". Is this a function?

Fixed.

> 
> > Source/WebCore/svg/SVGPointList.h:53
> > +    bool parse(const String& value);
> 
> StringView?

Fixed.

> 
> > Source/WebCore/svg/SVGPreserveAspectRatioValue.cpp:365
> > +        case SVG_PRESERVEASPECTRATIO_NONE:
> > +            return "none";
> > +        case SVG_PRESERVEASPECTRATIO_XMINYMIN:
> > +            return "xMinYMin";
> > +        case SVG_PRESERVEASPECTRATIO_XMIDYMIN:
> > +            return "xMidYMin";
> > +        case SVG_PRESERVEASPECTRATIO_XMAXYMIN:
> > +            return "xMaxYMin";
> > +        case SVG_PRESERVEASPECTRATIO_XMINYMID:
> > +            return "xMinYMid";
> > +        case SVG_PRESERVEASPECTRATIO_XMIDYMID:
> > +            return "xMidYMid";
> > +        case SVG_PRESERVEASPECTRATIO_XMAXYMID:
> > +            return "xMaxYMid";
> > +        case SVG_PRESERVEASPECTRATIO_XMINYMAX:
> > +            return "xMinYMax";
> > +        case SVG_PRESERVEASPECTRATIO_XMIDYMAX:
> > +            return "xMidYMax";
> > +        case SVG_PRESERVEASPECTRATIO_XMAXYMAX:
> > +            return "xMaxYMax";
> > +        case SVG_PRESERVEASPECTRATIO_UNKNOWN:
> > +            return "unknown";
> 
> Want some "_s" here I think. Unless the ASCIILiteral optimization is
> unimportant.

Fixed.

> 
> > Source/WebCore/svg/SVGPreserveAspectRatioValue.h:55
> > +    SVGPreserveAspectRatioValue(const StringView&);
> 
> StringView rather than const StringView&?

Fixed.

> 
> > Source/WebCore/svg/SVGPreserveAspectRatioValue.h:67
> > +    bool parse(const StringView&);
> 
> StringView rather than const StringView&?

Fixed.

> 
> > Source/WebCore/svg/SVGPreserveAspectRatioValue.h:78
> > +    template<typename CharacterType>
> > +    bool parseInternal(StringParsingBuffer<CharacterType>&, bool validate);
> 
> On one line?

Fixed. Drat. Thought I caught all of them.


> 
> > Source/WebCore/svg/SVGStringList.h:54
> > +    bool parse(const String& data, UChar delimiter);
> 
> StringView

Fixed.

> 
> > Source/WebCore/svg/SVGTransformList.cpp:59
> > +    for (const auto& transform : m_items)
> 
> I would have omitted the const here.

Fixed.

> 
> > Source/WebCore/svg/SVGTransformList.h:61
> > +    void parse(const String&);
> 
> StringView

Fixed,

> 
> > Source/WebCore/svg/SVGTransformable.cpp:100
> > +constexpr int requiredValuesForType[] =  {0, 6, 1, 1, 1, 1, 1};
> > +constexpr int optionalValuesForType[] =  {0, 0, 1, 1, 2, 0, 0};
> 
> Remove the extra space before "{"?
> 
> Not sure we should have removed static. Because constexpr does imply const,
> but not necessarily internal linkage. Unclear on the considerations.

Will add back.

> 
> > Source/WebCore/svg/SVGTransformable.h:37
> > +    static Optional<SVGTransformValue::SVGTransformType> parseTransformType(const StringView&);
> 
> StringView rather than const StringView&?

Fixed.

[Sam Weinig]

Created attachment 402938 [details]
Patch

[EWS]

Committed r263617: <https://trac.webkit.org/changeset/263617>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402938 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64843777>

[Darin Adler]

(In reply to Sam Weinig from comment #8)
> > > Source/WebCore/svg/SVGNumberList.h:54
> > > +    String valueAsString() const override;
> > 
> > final?
> 
> Made the class final, since it wasn't but should be. Is our style to use
> final on functions as well as the class?

It is.