Bug 58706 - DrawingBuffer::reset() should be faster
Summary: DrawingBuffer::reset() should be faster
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-15 16:03 PDT by Stephen White
Modified: 2011-04-18 11:23 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.21 KB, patch)
2011-04-15 16:08 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (10.93 KB, patch)
2011-04-16 09:49 PDT, Stephen White
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2011-04-15 16:03:41 PDT
DrawingBuffer::reset() is called when canvas.width or canvas.height is assigned.  It should not be ridiculously slow, especially in the case where the size is unchanged.
Comment 1 Stephen White 2011-04-15 16:08:18 PDT
Created attachment 89869 [details]
Patch
Comment 2 Kenneth Russell 2011-04-15 16:26:55 PDT
Comment on attachment 89869 [details]
Patch

I realize the current clearing code is pretty complicated but I think the intent was to leave the context's state untouched. Is this a goal? If we start using DrawingBuffer for the WebGL back buffer then no user-visible state changes can be allowed.
Comment 3 Zhenyao Mo 2011-04-15 17:44:22 PDT
Actually you should check the extension of GL_CHROMIUM_resource_safe.  If it exists, that means we are running on top of command buffer, and renderbuffers/textures have already been initialized, so you don't need to do it here again.

If the extension does not exist, then do the cleaning.  Ken is right, since Chris Marrin plans to use DrawingBuffer as WebGL back buffer, you should not make user-visible state changes, i.e., leave the original code un-touched.  That path won't be excercised in chrome anyway.

Otherwise, the patch looks good.

One thing I was thinking when I was looking through the change: can we push all the drawing buffer creation as one SUPER command for command buffer?  Note that we have to checkFramebufferStatus, which requires a sync.
Comment 4 Stephen White 2011-04-16 09:05:41 PDT
(In reply to comment #2)
> (From update of attachment 89869 [details])
> I realize the current clearing code is pretty complicated but I think the intent was to leave the context's state untouched. Is this a goal? If we start using DrawingBuffer for the WebGL back buffer then no user-visible state changes can be allowed.

It's not a goal for canvas2D.  To be honest, I'm not even really sure it's worth having a class that's shared between Canvas and WebGL here -- the goals are divergent in a lot of ways (state management, resource safety, etc).  But I'll put it back the way it was.
Comment 5 Stephen White 2011-04-16 09:23:28 PDT
(In reply to comment #3)
> Actually you should check the extension of GL_CHROMIUM_resource_safe.  If it exists, that means we are running on top of command buffer, and renderbuffers/textures have already been initialized, so you don't need to do it here again.

No, in the case that the size hasn't changed, we still need to glClear() them, even if we have GL_CHROMIUM_resource safe.  That's the main point of this change:  to skip the reallocation, and *only* do a glClear() in that case.

In the case that the size has changed, we *could* skip the glClear() if we had GL_CHROMIUM_resource_safe, but:

1)  I'd like to make this code future-proof against the case where we turn off resource safety for Canvas2D (for further speedups in the "size changed" case).

2)  The cost of an extra glClear() is tiny compared to the cost of memset() and (especially) texture upload.  For example, on my home machine, PCIe bandwidth = 3.2GB/sec (at best), and GPU memory bandwidth = 57.6GB/sec.  So even if the glClear() is redundant (for now) in the "size changed" case, the overhead is minimal.

> If the extension does not exist, then do the cleaning.  Ken is right, since Chris Marrin plans to use DrawingBuffer as WebGL back buffer, you should not make user-visible state changes, i.e., leave the original code un-touched.  That path won't be excercised in chrome anyway.

No, you miss the point.  I *want* it to be exercised for chrome (see above).  For example, this change gives a ~10X speedup on http://masterofthewebgame.com/.

> 
> Otherwise, the patch looks good.
> 
> One thing I was thinking when I was looking through the change: can we push all the drawing buffer creation as one SUPER command for command buffer?  Note that we have to checkFramebufferStatus, which requires a sync.

I suppose we could, but honestly, the cost of reallocating a texture (and that texture upload) likely dwarfs the cost of the sync.
Comment 6 Stephen White 2011-04-16 09:49:24 PDT
Created attachment 89925 [details]
Patch
Comment 7 Eric Seidel (no email) 2011-04-18 10:06:02 PDT
Comment on attachment 89925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89925&action=review

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:219
> +    // Initialize renderbuffers (depth/stencil).

This seems like a good sign that the block which follows should be its own helper method.
Comment 8 Kenneth Russell 2011-04-18 11:01:01 PDT
Comment on attachment 89925 [details]
Patch

Looks good to me. Thanks for your re-consideration of state changes in the context during the resize operation. I think it will be useful to be able to potentially reuse this class for all of the layer types which present GraphicsContext3D-rendered output to the compositor.
Comment 9 Stephen White 2011-04-18 11:21:54 PDT
Committed r84163: <http://trac.webkit.org/changeset/84163>
Comment 10 Stephen White 2011-04-18 11:23:05 PDT
(In reply to comment #7)
> (From update of attachment 89925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89925&action=review
> 
> > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:219
> > +    // Initialize renderbuffers (depth/stencil).
> 
> This seems like a good sign that the block which follows should be its own helper method.

Good point.  Will clean that up in a followup CL.