Bug 57269 – REGRESSION (r82151): Every WKView paint now makes an extra copy of the painted bits

• Component: Platform
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

r82151 changed ShareableBitmap::paint to use CGBitmapContextCreateImage in its implementation, rather than creating an image using a custom data provider. On Mac, CGBitmapContextCreateImage uses a copy-on-write implementation, but on Windows it just copies the data. This means that we're now making an extra copy of every pixel that we paint into a WKView.

This is likely a performance regression, though that has not been confirmed.

[Adam Roben (:aroben)]

<rdar://problem/9198788>

[Adam Roben (:aroben)]

It has now been confirmed that r82151 is a performance regression. Once upon a time we had a policy of rolling out performance regressions. I guess that's no longer the policy.

[Adam Roben (:aroben)]

Created attachment 88643 [details]
Patch for performance testing

Here's a patch that should get rid of the extra copy. I haven't performance tested it, though.

[Steve Falkenburg]

This patch is a 5% speed-up on iBench HTML cached.

[Adam Roben (:aroben)]

Comment on attachment 88643 [details]
Patch for performance testing

I don't think this patch is safe. If someone modifies the ShareableBitmap after the CGImageRef has been made but before the newMapping has been written into, the newMapping will see the modifications. This is exactly what we're trying to avoid. (And since newMapping should never be written into, we're basically never safe.)

This is just how copy-on-write views work in Windows. If I have two views of the same mapping, A and B, and A has FILE_MAP_WRITE and B has FILE_MAP_COPY, B will see writes to A until B is written into. From then on B will be operating on its own copy that is insulated from A.

[Adam Roben (:aroben)]

(In reply to comment #5)
> (From update of attachment 88643 [details])
> I don't think this patch is safe. If someone modifies the ShareableBitmap after the CGImageRef has been made but before the newMapping has been written into, the newMapping will see the modifications. This is exactly what we're trying to avoid. (And since newMapping should never be written into, we're basically never safe.)
> 
> This is just how copy-on-write views work in Windows. If I have two views of the same mapping, A and B, and A has FILE_MAP_WRITE and B has FILE_MAP_COPY, B will see writes to A until B is written into. From then on B will be operating on its own copy that is insulated from A.

I guess we could use A for the CGImageRef and use B for the ShareableBitmap from then on. The CGImageRef can't be written to, so that should work.

[Adam Roben (:aroben)]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 88643 [details] [details])
> > I don't think this patch is safe. If someone modifies the ShareableBitmap after the CGImageRef has been made but before the newMapping has been written into, the newMapping will see the modifications. This is exactly what we're trying to avoid. (And since newMapping should never be written into, we're basically never safe.)
> > 
> > This is just how copy-on-write views work in Windows. If I have two views of the same mapping, A and B, and A has FILE_MAP_WRITE and B has FILE_MAP_COPY, B will see writes to A until B is written into. From then on B will be operating on its own copy that is insulated from A.
> 
> I guess we could use A for the CGImageRef and use B for the ShareableBitmap from then on. The CGImageRef can't be written to, so that should work.

Trying this out now.

[Adam Roben (:aroben)]

Created attachment 92261 [details]
Make ShareableBitmap::makeCGImageCopy use a copy-on-write copy on Windows when possible

[Adam Roben (:aroben)]

Committed r85754: <http://trac.webkit.org/changeset/85754>