1476327 - Perf regression: Many small drawElements ranges

Status: RESOLVED FIXED

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

Description

[sebastien.videcoq]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36

Steps to reproduce:

open the sample provided on firefox ESR 52. mesure the frame rate.
open the sample provided on firefox ESR 60. mesure the frame rate.


Actual results:

there is performance regression. Profiling with gecko indicates a performance regression in index buffer validation (ValidateIndexedFetch). Seems to have been introduced with firefox 55.


Expected results:

WebGL performance should be identical or better

Comment 1

[sebastien.videcoq]

sample is too big to be attached : here is a link to google drive : https://drive.google.com/open?id=1jPHT8cRliiFS7ddcrDhCaG4tPN3F8nXz

Comment 2

[Gingerbread Man]

Can you reproduce the issue in a brand new profile with the latest Nightly?
https://support.mozilla.com/kb/profile-manager-create-and-remove-firefox-profiles
https://www.mozilla.org/firefox/nightly/all/

If yes, it would be helpful if you could find the exact regression range.
https://mozilla.github.io/mozregression/quickstart.html

Comment 3

[sebastien.videcoq]

Attachment: bisect.png

Comment 4

[sebastien.videcoq]

Yes the issue is reproducible with a new profile in latest nightly.

Not sure to understand why i got an error in bisect, but the regression seems to come from 2017-03-10 build. (see attached pic)

Comment 5

[Gingerbread Man]

(In reply to sebastien.videcoq from comment #4)
> (see attached pic)

Once the bisection completes,
1. In the "Bisection progress" section, left-click the last item ("Tested mozilla-central build: 2017-03-09") to select it.
2. In the "Bisection informations" section, click the "pushlog_url" link to open it.
3. Paste that link in a comment here.

Thank you for making the effort.

Comment 6

[sebastien.videcoq]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c40ca7a1bdd93632c6bdc5e23bd33d984d508b19&tochange=a8d497b09753c91783b68c5805c64f34a2f39629

Comment 7

[sebastien.videcoq]

kind remark for the one who will fix this regression :

We will get a huge performance impact on customer side due to this regression on ESR 60. This regression happens in index validation whereas most of our customers configurations (nVidia GPUs) supports GL_EXT_robustness. Can we move forward on such GPUs ?

In short, can we get rid of this index validation when it is not needed ?

Should i involve our contacts at nVidia or Angle side ?

Comment 8

[Sotaro Ikeda [:sotaro]]

:jgilbert, can you answer to comment 7?

Comment 9

[Julien Cristau [:jcristau] (back April 22)]

(I made a guess at the regressing bug based on the pushlog in comment 6)

Comment 10

[Kelsey Gilbert [:jgilbert]]

> Error: WebGL perf warning: [0x157646c40] New range #248: (0x1405, 26010440, 2142): 1426051 frame.js:8794:1
> Error: WebGL perf warning: [0x157646c40] New range #249: (0x1405, 23856432, 888): 1308025 frame.js:8797:1
> Error: WebGL perf warning: [0x157646c40] New range #250: (0x1405, 23863312, 2328): 1308761 frame.js:8812:1
> Error: WebGL perf warning: [0x157646c40] New range #251: (0x1405, 23646704, 2004): 1296662 frame.js:8818:1
> Error: WebGL perf warning: [0x157646c40] New range #252: (0x1405, 23620032, 2142): 1294990 frame.js:8821:1
> Error: WebGL perf warning: [0x157646c40] New range #253: (0x1405, 21466024, 888): 1176964 frame.js:8824:1
> Error: WebGL perf warning: [0x157646c40] New range #254: (0x1405, 21472904, 2328): 1177700 frame.js:8839:1
> Error: WebGL perf warning: [0x157646c40] New range #255: (0x1405, 21256296, 2004): 1165601 frame.js:8845:1
> Error: WebGL perf warning: [0x157646c40] New range #256: (0x1405, 21229624, 2142): 1163929 frame.js:8848:1
> Error: WebGL perf warning: [0x157646c40] Clearing mIndexRanges after exceeding 256. frame.js:8851:1
> Error: WebGL perf warning: [0x157646c40] New range #1: (0x1405, 19075616, 888): 1045903 frame.js:8851:1
> Error: WebGL perf warning: [0x157646c40] New range #2: (0x1405, 19082496, 2328): 1046639 frame.js:8866:1
> Error: WebGL perf warning: [0x157646c40] New range #3: (0x1405, 18865888, 2004): 1034540 frame.js:8872:1
> Error: WebGL perf warning: [0x157646c40] New range #4: (0x1405, 18839216, 2142): 1032868 frame.js:8875:1
> Error: WebGL perf warning: [0x157646c40] New range #5: (0x1405, 16685208, 888): 914842 frame.js:8878:1
> Error: WebGL perf warning: [0x157646c40] New range #6: (0x1405, 16692088, 2328): 915578 frame.js:8893:1
> Error: WebGL perf warning: [0x157646c40] New range #7: (0x1405, 16475480, 2004): 903479 frame.js:8899:1

This sure is the pessimal case!

Comment 11

[Kelsey Gilbert [:jgilbert]]

Worth noting that robust_buffer_access_behavior is not available on Mac.

Comment 12

[Kelsey Gilbert [:jgilbert]]

(In reply to sebastien.videcoq from comment #7)
> kind remark for the one who will fix this regression :
> 
> We will get a huge performance impact on customer side due to this
> regression on ESR 60. This regression happens in index validation whereas
> most of our customers configurations (nVidia GPUs) supports
> GL_EXT_robustness. Can we move forward on such GPUs ?
> 
> In short, can we get rid of this index validation when it is not needed ?
> 
> Should i involve our contacts at nVidia or Angle side ?

Unfortunately EXT/KHR_robustness don't provide sufficient guarantees, because they can allow reading out-of-bounds data, though they guarantee that the process won't crash. robust_buffer_access_behavior is required to skip index buffer validation, and that's detected when enabled. This should always be the case on D3D11 thus Windows, but it doesn't seem to be being activated right now. I'll look into that.

Separately, I wonder if we can add a trivial optimization where we store the max value in the index buffer, and so long as the vertex attrib arrays are all >= the max global index buffer element, we can skip all the checking. This is probably the case in general, since it should be uncommon to have the same index buffer used for many smaller attrib buffers.

If this is a static managed deployment of Firefox that does not run untrusted WebGL code, webgl.force-index-validation:-1 will force-disable index buffer validation.

Comment 13

[sebastien.videcoq]

(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> (In reply to sebastien.videcoq from comment #7)
> > kind remark for the one who will fix this regression :
> > 
> > We will get a huge performance impact on customer side due to this
> > regression on ESR 60. This regression happens in index validation whereas
> > most of our customers configurations (nVidia GPUs) supports
> > GL_EXT_robustness. Can we move forward on such GPUs ?
> > 
> > In short, can we get rid of this index validation when it is not needed ?
> > 
> > Should i involve our contacts at nVidia or Angle side ?
> 
> Unfortunately EXT/KHR_robustness don't provide sufficient guarantees,
> because they can allow reading out-of-bounds data, though they guarantee
> that the process won't crash. robust_buffer_access_behavior is required to
> skip index buffer validation, and that's detected when enabled. This should
> always be the case on D3D11 thus Windows, but it doesn't seem to be being
> activated right now. I'll look into that.
> 
> Separately, I wonder if we can add a trivial optimization where we store the
> max value in the index buffer, and so long as the vertex attrib arrays are
> all >= the max global index buffer element, we can skip all the checking.
> This is probably the case in general, since it should be uncommon to have
> the same index buffer used for many smaller attrib buffers.
> 
> If this is a static managed deployment of Firefox that does not run
> untrusted WebGL code, webgl.force-index-validation:-1 will force-disable
> index buffer validation.

Thanks for looking into this and for the detailed explanation. I think your additional proposal makes also sense. In our case it will always work.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: Test global upper bound for index buffer maxVertId.

Particularly in CAD applications, it's common to call drawElements many times on small
ranges of indices. This causes our naive maxVertId range cache to degenerate.

In most cases, the index buffer won't actually contain any indices outside the associated
VAO buffers' ranges. We should test first against this global upper-bound, only testing
for an exact maxVertId for the subrange if the upper-bound test fails.

Comment 15

[Phabricator Automation]

Comment on attachment 8995688 [details]
Test global upper bound for index buffer maxVertId.

Dzmitry Malyshau [:kvark] has approved the revision.

https://phabricator.services.mozilla.com/D2488

Comment 16

[Kelsey Gilbert [:jgilbert]]

The numbers:
> Nightly: 15 fps
> - Validation disabled: 17 fps
> - No validation, native GL: 21 fps
> With this fix: 17fps (same as validation disabled)
> ESR 52.9: 19 fps
> Edge: 19 fps
> Chrome: <doesn't render>

There's still room for improving draw call overhead, so I may be reusing this testcase. :)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/985e470b78d8
Test global upper bound for index buffer maxVertId. r=kvark

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/985e470b78d8

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This patch looks scary-big to me. What are your thoughts on uplifts here?

Comment 20

[Kelsey Gilbert [:jgilbert]]

We can try taking the RBAB-on-Windows patch there instead, which should also fix this.

This patch does do some code movement, but a decent amount in required since we need to support checking the upper-bound first, instead of returning an absolute bound that we can test later.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Relying on RBAB leaves OSX affected by this issue, since OSX doesn't support RBAB.

OSX will probably need to wait for ESR, unless we get more compelling pressure. It's a bad, but not severe perf hit, for a relatively narrow use-case, and no one hit it until this bug, as far as I've seen.

It might be worth trying to uplift to Beta, if it grafts cleanly.

Comment 22

[Kelsey Gilbert [:jgilbert]]

Actually we're already shipping this in Release, so it's easiest to let this ride the trains, I think.
We'll uplift the ANGLE+RBAB patch in bug 1477817 instead.