Bug 215150

Created attachment 405965 [details] Patch

Why do we need to do this for ImageBuffer? IOSurface::moveToPool() is called from a single place which is RemoteLayerBackingStore::Buffer::discard(). The RenderLayers are backed by IOSurfaces. While scrolling many RenderLayers are invalidated and painted. I think some of them are destroyed but many of them are recycled. The good thing about about RenderLayers is their sizes are almost always the same 512x512, I think. So it makes sense to recycle their IOSurfaces because a match will be found with a high probability. But I am not sure about the canvas and filters. Do we have the same pattern: 1. Create an IOSurface with size = s 2. Use the IOSurface 3. Destroy the IOSurface 4. Create another IOSurface with size = s We need to prove this case happens so often and we need to get measurements for how much this lazy destroying can affect the memory allocation.

(In reply to Said Abou-Hallawa from comment #2) > Why do we need to do this for ImageBuffer? > > IOSurface::moveToPool() is called from a single place which is > RemoteLayerBackingStore::Buffer::discard(). The RenderLayers are backed by > IOSurfaces. While scrolling many RenderLayers are invalidated and painted. I > think some of them are destroyed but many of them are recycled. The good > thing about about RenderLayers is their sizes are almost always the same > 512x512, I think. So it makes sense to recycle their IOSurfaces because a > match will be found with a high probability. > > But I am not sure about the canvas and filters. Do we have the same pattern: > 1. Create an IOSurface with size = s > 2. Use the IOSurface > 3. Destroy the IOSurface > 4. Create another IOSurface with size = s > > We need to prove this case happens so often and we need to get measurements > for how much this lazy destroying can affect the memory allocation. The scenario I was thinking of is CSS/SVG filter rendering with Accelerated rendering mode plus animation. In accelerated rendering mode where we use ImageBufferIOSurfaceBackend, when animating the filters, since CSS filter will be destroyed and recreated on each new draw, the ImageBuffer will have same life cycle as CSS filters, so does the IOSurface. That's why I was thinking doing so makes sense.

Comment on attachment 405965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405965&action=review > Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:110 > + IOSurface::moveToPool(WTFMove(m_surface)); Is there any guarantee m_surface is not null? ImageBufferIOSurfaceBackend::sinkIntoNativeImage() seems to move m_surface for instance

<rdar://problem/66872961>

(In reply to youenn fablet from comment #4) > Comment on attachment 405965 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405965&action=review > > > Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:110 > > + IOSurface::moveToPool(WTFMove(m_surface)); > > Is there any guarantee m_surface is not null? > ImageBufferIOSurfaceBackend::sinkIntoNativeImage() seems to move m_surface > for instance I guess we can check before throwing an IOSurface back to the pool. We only do so when m_surface is non-null.

(In reply to guowei_yang from comment #6) > (In reply to youenn fablet from comment #4) > > Comment on attachment 405965 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=405965&action=review > > > > > Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:110 > > > + IOSurface::moveToPool(WTFMove(m_surface)); > > > > Is there any guarantee m_surface is not null? > > ImageBufferIOSurfaceBackend::sinkIntoNativeImage() seems to move m_surface > > for instance > > I guess we can check before throwing an IOSurface back to the pool. We only > do so when m_surface is non-null. Sounds good

Comment on attachment 405965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405965&action=review > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). This line needs to be removed (Or add some tests).

Seems to be done already - https://searchfox.org/wubkat/rev/52d16c972a163b36fe64a99efb66af93c6e2f2b2/Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp#105

Marking this as `RESOLVED CONFIGURATION CHANGED`, please reopen, if something else is needed.