Bug 78426 – [Chromium] Avoid unnecessary memset in per-tile layer updater.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Reveman]

Use our own SkBitmap for which we manage the pixels instead of letting SkDevice construct a SkBitmap. This avoids an unnecessary memset otherwise done by SkDevice.

[David Reveman]

Created attachment 126651 [details]
Patch

[David Reveman]

Verified that this works both on Linux and Windows.

[David Reveman]

*** Bug 77897 has been marked as a duplicate of this bug. ***

[James Robinson]

This seems a little extreme, can't we just ask skia not to memset() and promise to set every pixel ourselves?

[David Reveman]

(In reply to comment #4)
> This seems a little extreme, can't we just ask skia not to memset() and promise to set every pixel ourselves?

AFAIK, there's no proper way to do that. We could pass isOpaque=true to the SkDevice constructor to avoid the memset but even though that might not have any bad side-effects in our case, we would be exploiting implementation details..

[James Robinson]

This is really really a Skia API question, so hopefully Mike or Stephen can chime in.  It seems excessive to require that in order to avoid a memset we have to take complete control over allocating and owning the memory for the bitmap.  TextureManager::memoryUseBytes() is a best-effort estimate for resource tracking, we haven't depending on it being exact up until now.

[David Reveman]

(In reply to comment #6)
> This is really really a Skia API question, so hopefully Mike or Stephen can chime in.  It seems excessive to require that in order to avoid a memset we have to take complete control over allocating and owning the memory for the bitmap.  TextureManager::memoryUseBytes() is a best-effort estimate for resource tracking, we haven't depending on it being exact up until now.

Sure, if the Skia API allows us do this then we should just use that.

As we already rely on the pixel format, row stride and hence also the size of the buffer to be of some specific values for our texture uploads to be correct I don't think it matters a huge deal if Skia owns the memory used for the bitmap or not.

Probably more appropriate to use SkBitmap::ComputeSize instead of TextureManager::memoryUseBytes() in this patch. I'll fix that for now.

[David Reveman]

Created attachment 126658 [details]
Patch

Use SkBitmap::ComputeSize

[Stephen White]

(In reply to comment #6)
> This is really really a Skia API question, so hopefully Mike or Stephen can chime in.  It seems excessive to require that in order to avoid a memset we have to take complete control over allocating and owning the memory for the bitmap.  TextureManager::memoryUseBytes() is a best-effort estimate for resource tracking, we haven't depending on it being exact up until now.

I think what you want is SkBitmap::allocPixels().  This will let skia take care of the size calculation, allocation and lifetime for you, while still avoiding the memset.

You could also keep the SkBitmap persistent between invocations of prepareRect() (which I assume are fairly frequent?) and avoid reallocating the bitmap if the size hasn't changed.  This would avoid both the memset() and the malloc() if the size is the same.

[David Reveman]

(In reply to comment #9)
> (In reply to comment #6)
> > This is really really a Skia API question, so hopefully Mike or Stephen can chime in.  It seems excessive to require that in order to avoid a memset we have to take complete control over allocating and owning the memory for the bitmap.  TextureManager::memoryUseBytes() is a best-effort estimate for resource tracking, we haven't depending on it being exact up until now.
> 
> I think what you want is SkBitmap::allocPixels().  This will let skia take care of the size calculation, allocation and lifetime for you, while still avoiding the memset.

Thanks! that will do it.

> 
> You could also keep the SkBitmap persistent between invocations of prepareRect() (which I assume are fairly frequent?) and avoid reallocating the bitmap if the size hasn't changed.  This would avoid both the memset() and the malloc() if the size is the same.

The malloc doesn't seem to cost much on the platforms where I've tested this so I'm avoiding that optimization for now as it might just a have negative effect on the memory pressure.

[David Reveman]

Created attachment 126682 [details]
Patch

Use allocPixels()

[James Robinson]

We used to preserve the buffer across frames for layers, but decided the memory cost wasn't worth it.  I think our allocator should perform pretty well for these sorts of allocations.

[David Reveman]

We could potentially use stack memory for this after landing https://bugs.webkit.org/show_bug.cgi?id=72752. On the other hand we're also working on painting directly to shared memory.

[Stephen White]

Comment on attachment 126682 [details]
Patch

Nice!  r=me

[WebKit Review Bot]

Comment on attachment 126682 [details]
Patch

Clearing flags on attachment: 126682

Committed r107515: <http://trac.webkit.org/changeset/107515>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mike Reed]

Alternatively, if you pass 'true' for the 4th (implicit) parameter to SkDevice's constructor, then you get two benefits:
1. I won't call memset, since you've told me the device is opaque
2. You've told me the device is opaque, so any blits using that bitmap will be faster, since it is known to be opaque.

[David Reveman]

(In reply to comment #17)
> Alternatively, if you pass 'true' for the 4th (implicit) parameter to SkDevice's constructor, then you get two benefits:
> 1. I won't call memset, since you've told me the device is opaque
> 2. You've told me the device is opaque, so any blits using that bitmap will be faster, since it is known to be opaque.

It's not always opaque though. Tile contents can be translucent.

[Dana Jansens]

(In reply to comment #17)
> Alternatively, if you pass 'true' for the 4th (implicit) parameter to SkDevice's constructor, then you get two benefits:
> 1. I won't call memset, since you've told me the device is opaque
> 2. You've told me the device is opaque, so any blits using that bitmap will be faster, since it is known to be opaque.

Presumably we can get this 2nd win by setting isOpaque on the bitmap appropriately? We already have information if the layer is fully opaque - at least for image layers. I can put together a simple CL for this.

[David Reveman]

(In reply to comment #19)
> (In reply to comment #17)
> > Alternatively, if you pass 'true' for the 4th (implicit) parameter to SkDevice's constructor, then you get two benefits:
> > 1. I won't call memset, since you've told me the device is opaque
> > 2. You've told me the device is opaque, so any blits using that bitmap will be faster, since it is known to be opaque.
> 
> Presumably we can get this 2nd win by setting isOpaque on the bitmap appropriately? We already have information if the layer is fully opaque - at least for image layers. I can put together a simple CL for this.

I don't think these bitmaps are ever used for blits.