Bug 137701 – [Mac] Avoid unnecessary dictionary lookup in ImageSource::isSizeAvailable()

• Component: Platform
• Status: RESOLVED FIXED

[Chris Dumez]

The CG implementation of ImageSource::isSizeAvailable() currently looks up both the width and the height keys in the dictionary even though it can abort early (and return false) if the first key is not in the dictionary.
Aborting early here is a small win but ImageSource::isSizeAvailable() is called quite frequently during page loads.

[Chris Dumez]

Created attachment 239805 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 239805 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:-187
> -    // Ragnaros yells: TOO SOON! You have awakened me TOO SOON, Executus!

Hyatt/Tim will be very sad this comment is gone.

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:192
> +    return CFDictionaryGetValue(image0Properties.get(), kCGImagePropertyPixelWidth)
> +        && CFDictionaryGetValue(image0Properties.get(), kCGImagePropertyPixelHeight);

This is a behavior change; you're no longer considering the value of these properties. Is that OK?

[Chris Dumez]

Comment on attachment 239805 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:-187
>> -    // Ragnaros yells: TOO SOON! You have awakened me TOO SOON, Executus!
> 
> Hyatt/Tim will be very sad this comment is gone.

Oh sorry about that, I am happy to leave it :)

>> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:192
>> +        && CFDictionaryGetValue(image0Properties.get(), kCGImagePropertyPixelHeight);
> 
> This is a behavior change; you're no longer considering the value of these properties. Is that OK?

Oh, if I changed behavior, this was not intentional. All I did is remove casts from void* to CFNumberRef, right? Why does this impact behavior?

[Simon Fraser (smfr)]

Comment on attachment 239805 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:192
>>> +        && CFDictionaryGetValue(image0Properties.get(), kCGImagePropertyPixelHeight);
>> 
>> This is a behavior change; you're no longer considering the value of these properties. Is that OK?
> 
> Oh, if I changed behavior, this was not intentional. All I did is remove casts from void* to CFNumberRef, right? Why does this impact behavior?

You removed the result = widthNumber && heightNumber;

[Chris Dumez]

Created attachment 239810 [details]
Patch

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 239805 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239805&action=review
> 
> >>> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:192
> >>> +        && CFDictionaryGetValue(image0Properties.get(), kCGImagePropertyPixelHeight);
> >> 
> >> This is a behavior change; you're no longer considering the value of these properties. Is that OK?
> > 
> > Oh, if I changed behavior, this was not intentional. All I did is remove casts from void* to CFNumberRef, right? Why does this impact behavior?
> 
> You removed the result = widthNumber && heightNumber;

Right, but widthNumber and heightNumber are CFNumberRef. Those are pointers, right? (I don't know much about those types). My understanding is that now I am doing (void*) && (void*) instead of CFNumberRef && CFNumberRef. I thought this was equivalent.

[Simon Fraser (smfr)]

Comment on attachment 239810 [details]
Patch

You're right, I was confused.

[WebKit Commit Bot]

Comment on attachment 239810 [details]
Patch

Clearing flags on attachment: 239810

Committed r174695: <http://trac.webkit.org/changeset/174695>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 239810 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        This is a small win but ImageSource::isSizeAvailable() is called quite
> +        frequently during page loads

We could also cache this boolean as long as we come up with some way to invalidate the cache when we pump more data into the decoder.

[Darin Adler]

Comment on attachment 239810 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:193
> +    return CFDictionaryGetValue(image0Properties.get(), kCGImagePropertyPixelWidth)
> +        && CFDictionaryGetValue(image0Properties.get(), kCGImagePropertyPixelHeight);

Since we are just checking if the dictionary contains a value, there’s a chance that CFDictionaryContainsValue might be slightly faster than CFDictionaryGetValue.

[Chris Dumez]

Comment on attachment 239810 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        frequently during page loads
> 
> We could also cache this boolean as long as we come up with some way to invalidate the cache when we pump more data into the decoder.

Oh, the boolean is already cached in BitmapImage class. But every time there is new image data available and m_sizeAvailable is false, we end up calling ImageSource::isSizeAvailable(), which makes sense.

[Chris Dumez]

Comment on attachment 239810 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:193
>> +        && CFDictionaryGetValue(image0Properties.get(), kCGImagePropertyPixelHeight);
> 
> Since we are just checking if the dictionary contains a value, there’s a chance that CFDictionaryContainsValue might be slightly faster than CFDictionaryGetValue.

I guess you mean CFDictionaryContainsKey(). That's a good point, it is a bit cleaner.