Closed Bug 1640960 Opened 5 years ago Closed 4 years ago

Stop using instancing in WebRender

Categories

(Core :: Graphics: WebRender, enhancement)

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: kvark, Assigned: kvark)

References

Details

Attachments

(4 files, 2 obsolete files)

Always sort WR shader features
47 bytes, text/x-phabricator-request
Details | Review
Support storage buffers with non-instanced rendering
47 bytes, text/x-phabricator-request
Details | Review
Add non-instancing code path to WR
48 bytes, text/x-phabricator-request
Details | Review
Support OpenGL 3.2 and SSBO in WR renderer
48 bytes, text/x-phabricator-request
Details | Review

There is information telling that our use of instancing may not be most efficient: https://www.slideshare.net/DevCentralAMD/vertex-shader-tricks-bill-bilodeau
Slide 19 says "Don't use DrawInstanced for creating sprites on AMD or NV hardware".

What we could do instead is - provide the instance data in shader storage buffers. According to wiki, they are in OpenGL ES 3.1 core, and available as https://www.khronos.org/registry/OpenGL/extensions/ARB/ARB_shader_storage_buffer_object.txt extension elsewhere. Angle should not have a problem supporting this either, although I haven't checked specifically yet.

We'd then issue regular DrawIndexed calls instead of the instanced ones. The vertex shader would then compute something like uint instanceId = gl_VertexId / 4;, and fetch the instance data from the storage buffer.

Once problem with this approach is the index buffer. Unless "base vertex" semantics is supported (e.g. glDrawElementsBaseVertex), we'd need the index buffer to be big, and used for all draw calls, and we'll need to potentially cut batches that are too big and not fitting into the index buffer range.

A nice side effect from this change is that we'd no longer be blocked on bug 1636630 for implementing efficient draw calls.

Keywords: leave-open

we only draw quads, and for quads we have a fixed vertex buffer with positions.
If we get stop using instancing, we'll no longer have the luxury of 4 vertices there.
Given that they are trivial to compute, it seems simpler to just do that in the shader today.
So this PR is a required step on the way to instance-less rendering.

Assignee: nobody → dmalyshau
Status: NEW → ASSIGNED
Pushed by dmalyshau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ca1168e024b9 Remove aPosition from all shaders, except debug ones r=gw

I found it hard to understand the code that builds shader features,
and even harder to modify it with a new feature. This PR refactors the shader
building code by removing the macro and introducing a FeatureList abstraction, internally.
It also sorts the features on both ends (alternatively, we could use a set).

Attached file Fix gl_VertexID unresolved reference (obsolete) —
Regressions: 1652763
Regressions: 1541711
Attachment #9163496 - Attachment is obsolete: true
Pushed by dmalyshau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a23787eb4292 Always sort WR shader features r=jnicol,gw
Pushed by dmalyshau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/697243e0434b Remove aPosition from all shaders, except debug ones r=gw,jrmuizel

The first version of the change got backed out. I investigated the problem, which I concluded to be the driver bug on macOS/Intel, and included the workaround in the second version of the change that has just landed.

Flags: needinfo?(dmalyshau)

Andreea, can we back out https://phabricator.services.mozilla.com/D83391 again? I suspect it to be related to https://bugzilla.mozilla.org/show_bug.cgi?id=1653210

Flags: needinfo?(apavel)

Backed out as requested.
Backout link: https://hg.mozilla.org/integration/autoland/rev/1b693164de4389acfa4b73167b56ad65341de09f

Flags: needinfo?(apavel)
Regressions: 1653283
Regressions: 1653210
Regressions: 1652939
Pushed by dmalyshau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4b66ae956966 Remove aPosition from all shaders, except debug ones r=gw,jrmuizel

(In reply to Bogdan Tara[:bogdan_tara | bogdant] from comment #16)

https://hg.mozilla.org/mozilla-central/rev/4b66ae956966

I'm seeing bug 1653283 again in today's nightly, was it cause by this landing?

Flags: needinfo?(dmalyshau)

I'm looking into this now. If we don't find a quick and simple solution, we should back out the original patch.

Flags: needinfo?(dmalyshau)

Dave, would you be able to reproduce the issue on these two builds?

Both of these are produced by Firefox CI with my fix candidates (first and second).

Flags: needinfo?(dtownsend)

(In reply to Dzmitry Malyshau [:kvark] from comment #19)

Dave, would you be able to reproduce the issue on these two builds?

Both of these are produced by Firefox CI with my fix candidates (first and second).

Yes I can reproduce the issue with both builds.

Flags: needinfo?(dtownsend)

Alright, thank you for testing quickly!

Atila, could you help backing out https://hg.mozilla.org/mozilla-central/rev/4b66ae956966 please?
Looks like the driver bugs have no end :(

Flags: needinfo?(abutkovits)

So I will say that while I can't reproduce this on-demand it will show up after a bit of clicking between tabs. For the first test build that took longer than normal so maybe it happens less then, maybe it was a fluke. The second build I saw it very quickly and so stopped testing so no idea there.

Regressions: 1652823
No longer regressions: 1652823

(In reply to Dzmitry Malyshau [:kvark] from comment #21)

Alright, thank you for testing quickly!

Atila, could you help backing out https://hg.mozilla.org/mozilla-central/rev/4b66ae956966 please?
Looks like the driver bugs have no end :(

Backed out as requested.
Backout link: https://hg.mozilla.org/integration/autoland/rev/b56eb8203c9c12d5ef7a87fc4af7cab7226f836e

Flags: needinfo?(abutkovits)
Attachment #9163313 - Attachment is obsolete: true

this change adds a code path to avoid instancing.
It's not universally supported, unfortunately.
Also, OSMESA appears to produce corrupted visuals on this path:
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/OkjOmxCVSO6QaiNQaCc0SA/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Side note: we still need a plan on what to do on devices that support neither of base-instance or SSBO.

About this plan I mentioned in the previous comment - we discussed it with Glenn. The outcomes are:

  1. we still want to land SSBO support, even if optional. However, we need to figure out the problem with OSMESA first, ideally.
  2. to cover other platforms, we'll need to experiment with the other ways of providing the data, such as textures, or vertex attributes
Depends on: 1661117
Pushed by dmalyshau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8d61a17c6f6e Support storage buffers with non-instanced rendering r=gw

Backed out for wrench failures on shadow-transforms.yaml

backout: https://hg.mozilla.org/integration/autoland/rev/9336e818075218a89eaa854948062db24f249923

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=8d61a17c6f6e642b6ee4437c7ee526afc2d4087a&selectedTaskRun=eWdxoEWlQmSCFJpwAuI0hQ.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=321985704&repo=autoland&lineNumber=13541

[task 2020-11-16T23:49:36.325Z] REFTEST reftests/text/shadow-partial-glyph.yaml == reftests/text/shadow-partial-glyph-ref.yaml
[task 2020-11-16T23:49:36.354Z] REFTEST reftests/text/shadow-transforms.yaml == reftests/text/shadow-transforms.png
[task 2020-11-16T23:49:36.547Z] REFTEST TEST-UNEXPECTED-FAIL | reftests/text/shadow-transforms.yaml == reftests/text/shadow-transforms.png | image comparison, max difference: 1, number of differing pixels: 185 | 185 differences > 0 and <= 2 (allowed 120);

Flags: needinfo?(dmalyshau)
Pushed by dmalyshau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dbcc7614b8f0 Support storage buffers with non-instanced rendering r=gw
Regressions: 1677728

Backed out changeset dbcc7614b8f0 (Bug 1640960) for causing rendering issues on Linux (Bug 1677728) a=backout

Backout link: https://hg.mozilla.org/mozilla-central/rev/8b19c30190d5c1548231daf6c9d04c5bda309f0f

Thanks for the backout! I'm going to test this on Intel NV and possibly re-land in a disabled form with a pref.

Flags: needinfo?(dmalyshau)

I apologise for this late regression, seems like I linked this bug directly to the alert.

We are adding the "enable_instancing" flag, enabled by default. When it's not true,
we are duplicating the per-vertex data and issuing non-instanced draw calls.
This is currently regressing the Talos tests.

Pushed by dmalyshau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6d125833c567 Add non-instancing code path to WR r=gw

This is a small extract from a change I tried to land earlier.
It refactors the GL version queries, no functional changes here.

Pushed by dmalyshau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0137ced478f3 Support OpenGL 3.2 and SSBO in WR renderer r=gw

We dug more information about instancing, and apparently the data from AMD presentation is no longer reflecting what the HW does.
Today, both AMD and NV are packing multiple instances into a wavefront, so removing instancing would cause us a perf regression.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: