When investigating the code of another project, I realized that whenever we use an AcceleratedImageBuffer (IOSurface backend), the IOSurface was always destroyed with the destruction of ImageBuffer. However, I do believe it makes more sense to put the IOSurface back to surface pool once an ImageBuffer is destroyed. There is actually an existing function, IOSurface::moveToPool implemented, but never used in ImageBuffers. I propose an idea that, we move the IOSurface back to the surface pool in the destructor of ImageBufferIOSurfaceBackend. I am not sure if this change is sufficient, but the current code definitely didn't use IOSurface pool for recycling.
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).