Closed Bug 1014209 Opened 6 years ago Closed 6 years ago

Performance impact from enabling WEBGL_draw_buffers

Categories

(Core :: Canvas: WebGL, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(1 file, 1 obsolete file)

There's currently a negative performance impact if WEBGL_draw_buffers is enabled. For example, we do additional work in Clear(). This is unexpected and can cause frameworks like emscripten to unintentionally lower the performance of applications.
I've filed https://github.com/kripken/emscripten/issues/2373 about trying to deal with this.
What is the perf impact? Glancing through the code, I don't see anything obvious. I assume we're over-clearing somehow?
The drawBuffers call in clear() causes a flush
... what drawBuffers call in clear()?
Ooh, ok. That's not in webgl.clear, that's in *ClearScreen*. This is an easy fix, we just need to cache calls to DrawBuffers.
Attached patch Something like this could work (obsolete) — Splinter Review
Attachment #8428010 - Flags: feedback?(jgilbert)
It may also be that DrawBuffers() is just causing work that the Clear() would have caused anyways...
Comment on attachment 8428010 [details] [diff] [review]
Something like this could work

Review of attachment 8428010 [details] [diff] [review]:
-----------------------------------------------------------------

We probably shouldn't be doing the state introspection that we do here, but if it's "ok" for now, we can leave it.

::: content/canvas/src/WebGLContext.cpp
@@ +994,5 @@
>      bool initializeColorBuffer = 0 != (mask & LOCAL_GL_COLOR_BUFFER_BIT);
>      bool initializeDepthBuffer = 0 != (mask & LOCAL_GL_DEPTH_BUFFER_BIT);
>      bool initializeStencilBuffer = 0 != (mask & LOCAL_GL_STENCIL_BUFFER_BIT);
>      bool drawBuffersIsEnabled = IsExtensionEnabled(WebGLExtensionID::WEBGL_draw_buffers);
> +    bool saveDrawBuffers = false;

It's less that we need to save it, and more that we need to override it temporarily. `shouldOverrideDrawBuffers`?
Attachment #8428010 - Flags: feedback?(jgilbert) → feedback+
(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Comment on attachment 8428010 [details] [diff] [review]
> Something like this could work
> 
> Review of attachment 8428010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We probably shouldn't be doing the state introspection that we do here, but
> if it's "ok" for now, we can leave it.

What would your preferred implementation look like?
Flags: needinfo?(jgilbert)
I confirmed this fixes the bug I was looking at
Attachment #8428010 - Attachment is obsolete: true
Attachment #8429473 - Flags: review?(jgilbert)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> (In reply to Jeff Gilbert [:jgilbert] from comment #8)
> > Comment on attachment 8428010 [details] [diff] [review]
> > Something like this could work
> > 
> > Review of attachment 8428010 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > We probably shouldn't be doing the state introspection that we do here, but
> > if it's "ok" for now, we can leave it.
> 
> What would your preferred implementation look like?

We should just cache this stuff, instead of ever risking calls to glGet* functions. We don't need to do that here, though.
Flags: needinfo?(jgilbert)
Attachment #8429473 - Flags: review?(jgilbert) → review+
Blocks: 1014011
Blocks: 1015326
No longer blocks: 1014011
https://hg.mozilla.org/mozilla-central/rev/6d1aa5734dd2
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
We are hoping this to be uplifted to FFOS 1.4, since we are targeting to improve performance on that release. Is this change possible to be applied there?
blocking-b2g: --- → 1.4?
1.4+ blocks a blocker
blocking-b2g: 1.4? → 1.4+
You need to log in before you can comment on or make changes to this bug.