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)
Created attachment 212551 [details] Patch
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.
Created attachment 212732 [details] Patch
Something like the latest patch ? This is even better, it greatly improves performances
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().
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.
> 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?
(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
(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.
Created attachment 213362 [details] Patch
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();
Created attachment 213380 [details] Patch
Everything has been fixed
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).
Committed <http://trac.webkit.org/r156902>.