Closed Bug 1476327 Opened 2 years ago Closed 2 years ago

Perf regression: Many small drawElements ranges

Categories

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

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: sebastien.videcoq, Assigned: jgilbert)

References

()

Details

(Keywords: perf, regression, testcase, Whiteboard: [gfx-noted])

Attachments

(2 files)

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
sample is too big to be attached : here is a link to google drive : https://drive.google.com/open?id=1jPHT8cRliiFS7ddcrDhCaG4tPN3F8nXz
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
Has STR: --- → yes
Component: Untriaged → Canvas: WebGL
Flags: needinfo?(sebastien.videcoq)
Product: Firefox → Core
Attached image bisect.png
Flags: needinfo?(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)
(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.
Flags: needinfo?(sebastien.videcoq)
Has Regression Range: --- → yes
Flags: needinfo?(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 ?
:jgilbert, can you answer to comment 7?
Flags: needinfo?(jgilbert)
Priority: -- → P3
Whiteboard: [gfx-noted]
(I made a guess at the regressing bug based on the pushlog in comment 6)
> 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!
Assignee: nobody → jgilbert
Flags: needinfo?(jgilbert)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: WebGL performance regression → Perf regressions: Many small drawElements ranges
Summary: Perf regressions: Many small drawElements ranges → Perf regression: Many small drawElements ranges
Worth noting that robust_buffer_access_behavior is not available on Mac.
(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.
(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.
See Also: → 1477817
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 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
Attachment #8995688 - Flags: review+
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. :)
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/985e470b78d8
Test global upper bound for index buffer maxVertId. r=kvark
https://hg.mozilla.org/mozilla-central/rev/985e470b78d8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
This patch looks scary-big to me. What are your thoughts on uplifts here?
Flags: needinfo?(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.
Flags: needinfo?(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.
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.
See Also: → 1482289
You need to log in before you can comment on or make changes to this bug.