Closed Bug 1330022 Opened 3 years ago Closed 3 years ago

16.4% of CPU time in rendering loop for a WebGL 2 demo is lost in gl.vertexAttribPointer() calls.

Categories

(Core :: Canvas: WebGL, defect, P3)

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: jujjyl, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(3 files)

Benchmarking a WebGL 2 demo from a partner,

after disabling ANGLE (to sidestep bug 1329983),
and no-opping FBO completeness checks (to sidestep bug 1329815),
and ignoring slow uniform uploads (bug 1329988 and https://bugs.chromium.org/p/angleproject/issues/detail?id=1694),

the remaining single biggest bottleneck in the application are calls to gl.vertexAttribPointer(), which takes up about 16.4% of total CPU execution time. This sounds a bit high.

Captured a geckoprofile of samples inside gl.vertexAttribPointer(), see the attachment. The node GLContext::fVertexAttribPointer() corresponds to time spent in the NVidia driver nvoglv64.dll. That is 27.2% of the total time spent in the function, which leaves 72.8% of the time inside the browser (roughly 3.67x more time compared to native)

I wonder if the attached callstack would highlight ideas for optimization opportunities?
On OS X, gl.vertexAttribPointer() is at the second highest spot in rendering, 6.7% of total execution time, so quite a bit less than on Windows.
They must be using it a lot. Are they not using VAOs?

Our validation shouldn't be particularly heavy here. (there's no O(N) to fix)

There are a couple of disparate switch statements we can probably merge for perf.
OS: Windows 10 → Unspecified
Priority: -- → P3
Hardware: x86_64 → Unspecified
Whiteboard: gfx-noted
Oops, wrong bug. Disregard Comment #3.
Flags: needinfo?(jujjyl)
Hey, sorry for the delay, I was travelling offsite for two weeks and didn't have time to test this before. I am now getting 404 on these when I click on the [B] link in the "Windows 8 x64 opt" line and then go to the build at bottom left, I wonder if the try builds have expired or something like that?

In any case, the changes seem to simplify the code, I suppose no harm in landing them in any case?
Flags: needinfo?(jujjyl)
Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

https://reviewboard.mozilla.org/r/110914/#review112476

Got a few minor concenrs and a question

::: dom/canvas/WebGLContextVertices.cpp:338
(Diff revision 1)
> +    case LOCAL_GL_UNSIGNED_SHORT:
> +        typeAlignment = 2;
> +        break;
> +
> +    case LOCAL_GL_FLOAT:
> +        if (isFuncInt) {

we might want to check for `normalized==false` here as well

::: dom/canvas/WebGLContextVertices.cpp:416
(Diff revision 1)
> +    }
> +
> +    ////
> +
> +    gl->MakeCurrent();
> +    if (isFuncInt) {

we may also want to check for (isFuncInt && normalized) to be invalid

::: dom/canvas/WebGLContextVertices.cpp:420
(Diff revision 1)
> +    gl->MakeCurrent();
> +    if (isFuncInt) {
> +        gl->fVertexAttribIPointer(index, size, type, stride,
> +                                  reinterpret_cast<void*>(byteOffset));
> +    } else {
> -    gl->fVertexAttribPointer(index, size, type, normalized, stride,
> +        gl->fVertexAttribPointer(index, size, type, normalized, stride,

any reason we call `fVertexAttribPointer` here directly instead of doing `vd.DoVertexAttribPointer(..)`?
Attachment #8835228 - Flags: review?(kvark) → review-
Comment on attachment 8835229 [details]
Bug 1330022 - Remove trivial ValidateUniformMatrixTranspose virtual. -

https://reviewboard.mozilla.org/r/110916/#review112480

looks good!
Attachment #8835229 - Flags: review?(kvark) → review+
Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

https://reviewboard.mozilla.org/r/110914/#review112690
Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

https://reviewboard.mozilla.org/r/110914/#review112692
Attachment #8835228 - Flags: review?(jgilbert)
Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

https://reviewboard.mozilla.org/r/110914/#review112694
Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

https://reviewboard.mozilla.org/r/110914/#review112476

> we might want to check for `normalized==false` here as well

normalized only changes the range of the resulting float, not the type.
A non-normalized GL_UNSIGNED_BYTE specified with VertexAttribPointer is a float in the range [0.0, 255.0], whereas with VertexAttribPointerI it's a uint in the range [0,255].

> we may also want to check for (isFuncInt && normalized) to be invalid

There's not much point in adding an assert here.

> any reason we call `fVertexAttribPointer` here directly instead of doing `vd.DoVertexAttribPointer(..)`?

That just moves code around without simplifying.
Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

https://reviewboard.mozilla.org/r/110914/#review112696
Attachment #8835228 - Flags: review?(jgilbert)
Attachment #8835228 - Flags: review- → review?(kvark)
Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

https://reviewboard.mozilla.org/r/110914/#review112860
Attachment #8835228 - Flags: review?(kvark) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0528322db042
Centralize VertexAttrib[I]Pointer calls to improve perf. - r=kvark
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd91d6b1b500
Remove trivial ValidateUniformMatrixTranspose virtual. - r=kvark
Backed out for failing test_conformance__more__functions__vertexAttribPointerBadArgs.htm:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8675b60fad97dd1c1c8e999b3dd5f7c7aa9582dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fa758c8d307b7ecf204439b4a0bf08b53927aa

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cd91d6b1b500db91250cd5d87a3c5afb7d7f6c1e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=76465569&repo=mozilla-inbound

[task 2017-02-10T21:59:52.864500Z] 21:59:52     INFO - TEST-START | dom/canvas/test/webgl-conf/generated/test_conformance__more__functions__vertexAttribPointerBadArgs.html
[task 2017-02-10T21:59:53.180278Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 1274: Error: WebGL warning: Exceeded 16 live WebGL contexts for this principal, losing the least recently used one.
[task 2017-02-10T21:59:53.180362Z] 21:59:53     INFO - WebGL(0x7f5b63dd6800)::ForceLoseContext
[task 2017-02-10T21:59:53.216679Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 1002: Error: WebGL warning: Exceeded 16 live WebGL contexts for this principal, losing the least recently used one.
[task 2017-02-10T21:59:53.216766Z] 21:59:53     INFO - WebGL(0x7f5b7a512800)::ForceLoseContext
[task 2017-02-10T21:59:53.254231Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 965: Error: WebGL warning: vertexAttribPointer: negative offset
[task 2017-02-10T21:59:53.254393Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 965: Error: WebGL warning: vertexAttribPointer: `stride` and `byteOffset` must satisfy the alignment requirement of `type`.
[task 2017-02-10T21:59:53.255337Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 965: Error: WebGL warning: vertexAttribPointer: negative or too large stride
[task 2017-02-10T21:59:53.255451Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 965: Error: WebGL warning: vertexAttribPointer: invalid element size
[task 2017-02-10T21:59:53.259130Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 965: Error: WebGL warning: vertexAttribPointer: `stride` and `byteOffset` must satisfy the alignment requirement of `type`.
[task 2017-02-10T21:59:53.259241Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 965: Error: WebGL warning: vertexAttribPointer: Bad `type`: TEXTURE_2D
[task 2017-02-10T21:59:53.262940Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 965: Error: WebGL warning: vertexAttribPointer: -1 is not a valid `index`. This value probably comes from a getAttribLocation() call, where this return value -1 means that the passed name didn't correspond to an active attribute in the specified program.
[task 2017-02-10T21:59:53.263500Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 965: Error: WebGL warning: vertexAttribPointer: `index` must be less than MAX_VERTEX_ATTRIBS.
[task 2017-02-10T21:59:53.266481Z] 21:59:53     INFO - JavaScript warning: http://mochi.test:8888/tests/dom/canvas/test/webgl-conf/checkout/conformance/more/util.js, line 965: Error: WebGL warning: vertexAttribPointer: `index` must be less than MAX_VERTEX_ATTRIBS.
[task 2017-02-10T21:59:53.275691Z] 21:59:53     INFO - TEST-INFO | started process screentopng
[task 2017-02-10T21:59:53.939726Z] 21:59:53     INFO - TEST-INFO | screentopng: exit 0
[task 2017-02-10T21:59:53.940671Z] 21:59:53     INFO - Buffered messages logged at 21:59:53
[task 2017-02-10T21:59:53.940773Z] 21:59:53     INFO - TEST-PASS | dom/canvas/test/webgl-conf/generated/test_conformance__more__functions__vertexAttribPointerBadArgs.html | A valid string reason is expected 
[task 2017-02-10T21:59:53.940860Z] 21:59:53     INFO - TEST-PASS | dom/canvas/test/webgl-conf/generated/test_conformance__more__functions__vertexAttribPointerBadArgs.html | Reason cannot be empty 
[task 2017-02-10T21:59:53.940917Z] 21:59:53     INFO - Buffered messages finished
[task 2017-02-10T21:59:53.941663Z] 21:59:53     INFO - TEST-UNEXPECTED-FAIL | dom/canvas/test/webgl-conf/generated/test_conformance__more__functions__vertexAttribPointerBadArgs.html | testVertexAttribPointerVBO 
[task 2017-02-10T21:59:53.942049Z] 21:59:53     INFO -     reportResults@dom/canvas/test/webgl-conf/mochi-single.html?checkout/conformance/more/functions/vertexAttribPointerBadArgs.html:22:7
[task 2017-02-10T21:59:53.942124Z] 21:59:53     INFO -     reportTestResultsToHarness@dom/canvas/test/webgl-conf/checkout/conformance/more/unit.js:939:5
[task 2017-02-10T21:59:53.942468Z] 21:59:53     INFO -     runTests@dom/canvas/test/webgl-conf/checkout/conformance/more/unit.js:229:5
[task 2017-02-10T21:59:53.942543Z] 21:59:53     INFO -     initTests@dom/canvas/test/webgl-conf/checkout/conformance/more/unit.js:960:5
[task 2017-02-10T21:59:53.943167Z] 21:59:53     INFO -     @dom/canvas/test/webgl-conf/checkout/conformance/more/unit.js:985:7
[task 2017-02-10T21:59:53.943248Z] 21:59:53     INFO -     EventListener.handleEvent*@dom/canvas/test/webgl-conf/checkout/conformance/more/unit.js:979:1
Flags: needinfo?(jgilbert)
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab9aea090d68
Centralize VertexAttrib[I]Pointer calls to improve perf. - r=kvark
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9027426615c
Remove trivial ValidateUniformMatrixTranspose virtual. - r=kvark
https://hg.mozilla.org/integration/mozilla-inbound/rev/214a39a8134d
Update test.
Flags: needinfo?(jgilbert)
Duplicate of this bug: 1326517
You need to log in before you can comment on or make changes to this bug.