[webkit-reviews] review granted: [Bug 125477] Allow ImageBuffer to re-use IOSurfaces : [Attachment 219792] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 125477: Allow ImageBuffer to re-use IOSurfaces
https://bugs.webkit.org/show_bug.cgi?id=125477

Attachment 219792: Patch
https://bugs.webkit.org/attachment.cgi?id=219792&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219792&action=review


r=me

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:116
> +    return result;

You gotta move result, too, or no optimization is guaranteed.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:171
> +    IntSize surfaceSize = IntSize(IOSurfaceGetWidth(ioSurface),
IOSurfaceGetHeight(ioSurface));

"= IntSize"

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:192
> +    // Clear opportunistically so CG has more time to carry it out

Let's put a period at the end of this, to make it a sentence.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:101
> +    // If we find an acceptable surface, this function removes it from the
cache as
> +    // well as placing it in the out parameter

Period at end of sentence, please.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:102
> +    bool tryTakeAcceptableCachedSurface(const IntSize&, CGColorSpaceRef,
bool needExactSize, IOSurfaceAndContextWithCreationParams& outInfo);

Let's call this tryTakeFromCache, since it's a wrapper around takeFromCache.

> PerformanceTests/Canvas/reuse.html:7
> +var numCreated = 10000;

The period of this benchmark seems so big that no reasonably-sized cache will
ever get a hit. So, this benchmark is artificially biased toward giant caches,
and it doesn't test much in the way of eviction algorithm.

Maybe just change it to fewer canvases.