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

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied 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 219712: Patch
https://bugs.webkit.org/attachment.cgi?id=219712&action=review

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


> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:67
> +    RetainPtr<CFDictionaryRef> dict = adoptCF(CFDictionaryCreate(0, keys,
values, 6, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

kNumCreationParameters

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:68
> +    for (unsigned i = 0; i < 6; i++)

kNumCreationParameters

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:86
>
+ImageBufferBackingStoreCache::IOSurfaceInfoWithCreationParams::IOSurfaceInfoWi
thCreationParams(IOSurfaceInfo&& info, CGColorSpaceModel colorSpace)
> +    : m_prev(nullptr)
> +    , m_next(nullptr)
> +    , m_info(std::forward<IOSurfaceInfo>(info))
> +    , m_colorSpace(colorSpace)
> +{
> +}

This doesn't look like the right way to do the C++ move idiom.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:112
> +    auto* toAdd = new IOSurfaceInfoWithCreationParams(info);

Should be "auto".

It's not good to take a const& and pass it to a constructor intended to do a
move operation. "const&" means "don't modify my data" and "move" means "take
all my data away".

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:133
> +    if (didRemoveLastElementInList)
> +	   *didRemoveLastElementInList = iter->value.isEmpty();
> +    else
> +	   m_cachedSurfaces.remove(iter);

This out parameter is pretty awkward. Can you arrange for the delete function
to remove the empty list? That's part of the job of having a helper function --
we want a single place to manage the memory.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:137
> +bool ImageBufferBackingStoreCache::findAcceptableCachedSurface(const
IntSize& size, CGColorSpaceModel colorSpace, bool needExactSize,
IOSurfaceInfoWithCreationParams& outInfo)

I still think this should be called "take", since it removes an item from the
cache.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:145
> +	       outInfo = deleteFromCache(i, info, nullptr);

I would call this "take". A C++ programmer should instinctively fear and loathe
any line of code that says "x = delete y".

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:200
> +    // Evict

This code right below this comment doesn't do what this comment says.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:224
> +    Vector<CachedSurfaceKey> bucketsToRemove;
> +    bucket = m_cachedSurfaces.begin();
> +    while (m_pixelsCached + surfaceArea > kMaxPixelsCached) {
> +	   bool shouldRemoveEmptyEntry;
> +	   deleteFromCache(bucket, bucket->value.head(),
&shouldRemoveEmptyEntry);
> +	   if (shouldRemoveEmptyEntry)
> +	       bucketsToRemove.append(bucket->key);
> +	   ++bucket;
> +	   if (bucket == m_cachedSurfaces.end()) {
> +	       clearEmptyBuckets(bucketsToRemove);
> +	       bucket = m_cachedSurfaces.begin();
> +	   }
> +    }
> +    clearEmptyBuckets(bucketsToRemove);

This is too much code and CPU time for a not-very-good eviction algorithm.
Let's at least get the benefits of a not-very-good eviction algorithm, and keep
things simple.

Let's go with removing begin().head() repeatedly.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:62
> +    IOSurfaceInfo getIOSurface(const IntSize&, RetainPtr<CGColorSpaceRef>,
bool needExactSize);

I'd call this "getOrAllocate".

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:63
> +    void releaseIOSurface(RetainPtr<IOSurfaceRef>);

I would call this "deallocate". No need to mention IOSurface in the name thrice
-- it's already in the argument signature part of the name, and in the class
name. "deallocate" helps indicate that things not deallocated will cause memory
leaks in the cache. It's important to call that out because most caches don't
work that way.