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

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P3)

Description

[Jukka Jylänki]

Attachment: glvertexattribpointer.png

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?

Comment 1

[Jukka Jylänki]

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.

Comment 2

[Kelsey Gilbert [:jgilbert]]

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.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Please retest with this build:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=67618658d398ae4647c8337b1b8ef31172988c64

Comment 4

[Kelsey Gilbert [:jgilbert]]

Oops, wrong bug. Disregard Comment #3.

Comment 5

[Kelsey Gilbert [:jgilbert]]

This might help:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=0626bdec105e3c40e26d0ed1d8ba659cd0c1b8fe

Please compare against its base:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03fab9c985a831c4e5520d60c3dbec3a91c4b592

Comment 6

[Jukka Jylänki]

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?

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

Review commit: https://reviewboard.mozilla.org/r/110914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110914/

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1330022 - Remove trivial ValidateUniformMatrixTranspose virtual. -

Removing a file reshuffled our unified builds, so this includes the fixes that required as well.

Review commit: https://reviewboard.mozilla.org/r/110916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110916/

Comment 9

[Dzmitry Malyshau [:kvark]]

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(..)`?

Comment 10

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8835229 [details]
Bug 1330022 - Remove trivial ValidateUniformMatrixTranspose virtual. -

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

looks good!

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

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

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

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

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

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

Comment 14

[Kelsey Gilbert [:jgilbert]]

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 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

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

Comment 16

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab9aea090d68
https://hg.mozilla.org/mozilla-central/rev/a9027426615c
https://hg.mozilla.org/mozilla-central/rev/214a39a8134d