Bug 121899 – Optimize strings copies in srcset parser

• Component: Images
• Status: RESOLVED FIXED

[Romain Perier]

Images URLs are copied for each loop iteration during the parsing algorithm. This is important to avoid sharing the same buffer with "srcsetattribute" which might be removed after the attribute has been processed (outside of "bestFitSourceForImageAttributes")
A good optimization would be to use shadow copies during parsing and deep copies when the selection algorithm returns the good candidate (as "srcsetAttribute" is not out of scope, this string is valid as long as the function "bestFitSourceForImageAttributes" runs)

[Romain Perier]

Created attachment 212551 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 212551 [details]
Patch

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

I think that this is a useful thing to look into. r- for regressing src preformance though.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:393
> +        image.imageURL = StringImpl::createWithoutCopying(srcsetAttribute.characters() + imageURLStart, imageURLEnd - imageURLStart);

I'm not really a fan of passing unsafe string across functions. Can we just record the start and length of the URL as indexes, and only create a string when needed?

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:425
> +    return String(imageCandidates.last().imageURL.characters(), imageCandidates.last().imageURL.length());

This adds an extra copy in the common case where we have an src and no srcset.

[Romain Perier]

Created attachment 212732 [details]
Patch

[Romain Perier]

Something like the latest patch ? This is even better, it greatly improves performances

[Alexey Proskuryakov]

Comment on attachment 212732 [details]
Patch

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

Yes, this looks much better to me. A few questions/comment below:

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:311
>      bool operator==(const ImageWithScale& image) const

Is the operator== actually needed? I don't immediately see where it's used, although I could have easily overlooked that.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:418
> +        image.imageURLStart = 0;
> +        image.imageURLLength = 0;

Can we add a constructor to ImageWithScale instead?

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:433
> +    return imageCandidates.last().hasNoImage() ? srcAttribute : String(srcsetAttribute.characters() + imageCandidates.last().imageURLStart, imageCandidates.last().imageURLLength);

Please use srcsetAttribute.substring() instead. That's more readable, and could help future optimizations.

Also, I'd use a local variable for imageCandidates.last().

[Benjamin Poulain]

Comment on attachment 212732 [details]
Patch

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

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:303
> +    size_t imageURLStart;

String length are unsigned. You should have imageURLStart unsigned .

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:309
> +        return !imageURLStart && !imageURLLength;

Isn't !imageURLLength enough?

The method should probably be hasImageURL() and the caller use "!hasImage()".

>> Source/WebCore/html/parser/HTMLParserIdioms.cpp:433
>> -            return String(imageCandidates[i].imageURL);
>> +            return imageCandidates[i].hasNoImage() ? srcAttribute : String(srcsetAttribute.characters() + imageCandidates[i].imageURLStart, imageCandidates[i].imageURLLength);
>>      }
>> -    return String(imageCandidates.last().imageURL);
>> +    return imageCandidates.last().hasNoImage() ? srcAttribute : String(srcsetAttribute.characters() + imageCandidates.last().imageURLStart, imageCandidates.last().imageURLLength);
> 
> Please use srcsetAttribute.substring() instead. That's more readable, and could help future optimizations.
> 
> Also, I'd use a local variable for imageCandidates.last().

substring() would be better indeed. StringImpl::substring takes into account the 8/16 bits strings.
You could even use String::substringSharingImpl() here since srcsetAttribute will stay alive anyway.

[Alexey Proskuryakov]

> srcsetAttribute will stay alive anyway

What guaranteed this? E.g., can we have a dangling pointer for a while between changing the attribute via DOM and a style recalc?

[Romain Perier]

(In reply to comment #5)
> (From update of attachment 212732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212732&action=review
> 
> Yes, this looks much better to me. A few questions/comment below:
> 
> > Source/WebCore/html/parser/HTMLParserIdioms.cpp:311
> >      bool operator==(const ImageWithScale& image) const
> 
> Is the operator== actually needed? I don't immediately see where it's used, although I could have easily overlooked that.

Not sure, it was probably used for sorting ... but we use a "comp" function for this... not really clear. Removing it does not introduce regressions.
Good catch.

> 
> > Source/WebCore/html/parser/HTMLParserIdioms.cpp:418
> > +        image.imageURLStart = 0;
> > +        image.imageURLLength = 0;
> 
> Can we add a constructor to ImageWithScale instead?

sure, it's more readable and probably faster

> 
> > Source/WebCore/html/parser/HTMLParserIdioms.cpp:433
> > +    return imageCandidates.last().hasNoImage() ? srcAttribute : String(srcsetAttribute.characters() + imageCandidates.last().imageURLStart, imageCandidates.last().imageURLLength);
> 
> Please use srcsetAttribute.substring() instead. That's more readable, and could help future optimizations.

OK, makes sense
> 
> Also, I'd use a local variable for imageCandidates.last().

OK

[Benjamin Poulain]

(In reply to comment #7)
> > srcsetAttribute will stay alive anyway
> 
> What guaranteed this? E.g., can we have a dangling pointer for a while between changing the attribute via DOM and a style recalc?

Even if you were to unref srcsetAttribute, substringSharingImpl ref() the original string, you will not get a dangling pointer.

[Romain Perier]

Created attachment 213362 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 213362 [details]
Patch

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

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:306
> +    ImageWithScale() : imageURLStart(0), imageURLLength(0), scaleFactor(1.0) { }

I think that the preferred style would be the same as in non-inline case, with each initializer and each brace on its own line.

And we don't add .0 to literal values. Especially in this case, because 1.0 is a double, but scaleFactor is a float.

    ImageWithScale()
        : imageURLStart(0)
        , imageURLLength(0)
        , scaleFactor(1)
     {
     }

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:414
>          ImageWithScale image;
> -        image.imageURL = srcAttribute;
> -        image.scaleFactor = 1.0;
> -
>          imageCandidates.append(image);

This is getting somewhat convoluted. Please rename "image" to "srcPlaceholderImage" for a little more clarity.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:426
> +    const ImageWithScale &lastCandidate = imageCandidates.last();

The "&" should be attached to type name, 

const ImageWithScale& lastCandidate = imageCandidates.last();

[Romain Perier]

Created attachment 213380 [details]
Patch

[Romain Perier]

Everything has been fixed

[Alexey Proskuryakov]

Comment on attachment 213380 [details]
Patch

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

I'll just land with a style fix manually, the iterations for minor style fixes are getting annoying.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:312
> +        { 
> +        }

These should not be indented (were not in the snippet I posted).

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r156902>.