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

RESOLVED FIXED in Firefox 54

Status

()

Core
Canvas: WebGL
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: Jukka Jylänki, Assigned: jgilbert)

Tracking

(Blocks: 1 bug)

53 Branch
mozilla54
Points:
---

Firefox Tracking Flags

(firefox51 wontfix, firefox53 affected, firefox54 fixed)

Details

(Whiteboard: gfx-noted)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8825447 [details]
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?
(Reporter)

Comment 1

a year ago
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.
(Assignee)

Comment 2

a year ago
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.
status-firefox51: --- → wontfix
OS: Windows 10 → Unspecified
Priority: -- → P3
Hardware: x86_64 → Unspecified
Whiteboard: gfx-noted
Comment hidden (obsolete)
(Assignee)

Comment 4

a year ago
Oops, wrong bug. Disregard Comment #3.
Flags: needinfo?(jujjyl)
(Reporter)

Comment 6

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
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 10

a year ago
mozreview-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+
(Assignee)

Comment 11

a year ago
mozreview-review
Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

https://reviewboard.mozilla.org/r/110914/#review112690
(Assignee)

Comment 12

a year ago
mozreview-review
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)
(Assignee)

Comment 13

a year ago
mozreview-review
Comment on attachment 8835228 [details]
Bug 1330022 - Centralize VertexAttrib[I]Pointer calls to improve perf. -

https://reviewboard.mozilla.org/r/110914/#review112694
(Assignee)

Comment 14

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 15

a year ago
mozreview-review
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)
(Assignee)

Updated

a year ago
Attachment #8835228 - Flags: review- → review?(kvark)

Comment 16

a year ago
mozreview-review
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+

Comment 17

a year ago
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)
(Reporter)

Updated

a year ago
Blocks: 1207170

Comment 19

a year ago
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.

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab9aea090d68
https://hg.mozilla.org/mozilla-central/rev/a9027426615c
https://hg.mozilla.org/mozilla-central/rev/214a39a8134d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

a year ago
Flags: needinfo?(jgilbert)
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1326517
You need to log in before you can comment on or make changes to this bug.