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.
Created attachment 239805 [details] Patch
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?
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?
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;
Created attachment 239810 [details] Patch
(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.
Comment on attachment 239810 [details] Patch You're right, I was confused.
Comment on attachment 239810 [details] Patch Clearing flags on attachment: 239810 Committed r174695: <http://trac.webkit.org/changeset/174695>
All reviewed patches have been landed. Closing bug.
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.
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.
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.
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.