1496168 - ANGLE generates mipmaps for WebRender textures unconditionally

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P2)

Description

[Bobby Holley (:bholley)]

This causes unnecessary memory usage, and may impose a performance penalty as well.

I verified with RenderDoc that all the large WebRender textures have ~12 mipmap levels. Using the stack in bug 1495977, I poked around in the ANGLE source and found the following:

* TextureD3D_2DArray::initializeStorage() calls createCompleteStorage() [1]
* createCompleteStorage() determines mipmap levels by calling creationLevels() [2]
* creationLevels() generates maximum levels if all the dimension are power-of-two [3]

[1] https://searchfox.org/mozilla-central/rev/3c85ea2f8700ab17e38b82d77cd44644b4dae703/gfx/angle/checkout/src/libANGLE/renderer/d3d/TextureD3D.cpp#3185
[2] https://searchfox.org/mozilla-central/rev/3c85ea2f8700ab17e38b82d77cd44644b4dae703/gfx/angle/checkout/src/libANGLE/renderer/d3d/TextureD3D.cpp#1400
[3] https://searchfox.org/mozilla-central/rev/3c85ea2f8700ab17e38b82d77cd44644b4dae703/gfx/angle/checkout/src/libANGLE/renderer/d3d/TextureD3D.cpp#383

Comment 1

[Bobby Holley (:bholley)]

Jeff, am I missing something here? If not, what shall we do about it?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

The typical way to avoid this problem is to use glTexStorage2D instead of glTexImage2d. I'm not certain this avoids the problem in ANGLE, but if it doesn't ANGLE could be adjusted to fix that.

See: https://github.com/servo/webrender/issues/2124

Comment 3

[Glenn Watson [:gw]]

Isn't TexStorage2D only available in GL4+ though? (although perhaps the extension version is widely available in GL3?)

Comment 4

[Bobby Holley (:bholley)]

If we've otherwise managed to stay with GL3, this doesn't seem like a very compelling forcing function to change that (and rewire our texture setup from mutable to immutable). The MVP is going to go through ANGLE, so we just need a way to tell ANGLE not to do what it's doing.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to Glenn Watson [:gw] from comment #3)
> Isn't TexStorage2D only available in GL4+ though? (although perhaps the
> extension version is widely available in GL3?)

Yes and yes.

Comment 6

[Bobby Holley (:bholley)]

Ah, looks like we use one extension already [1].

So we could go that route, assuming we think the extension is widely-available enough. Though our current render target pool setup relies on resizing textures, which presumably wouldn't work with immutable storage. I'm not sure of how much it would cost us to give that up, but fixing ANGLE still seems easier.

[1] https://searchfox.org/mozilla-central/rev/3c85ea2f8700ab17e38b82d77cd44644b4dae703/gfx/webrender/src/renderer.rs#1557

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bobby Holley (:bholley) from comment #6)
> Ah, looks like we use one extension already [1].
> 
> So we could go that route, assuming we think the extension is
> widely-available enough. Though our current render target pool setup relies
> on resizing textures, which presumably wouldn't work with immutable storage.
> I'm not sure of how much it would cost us to give that up, but fixing ANGLE
> still seems easier.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 3c85ea2f8700ab17e38b82d77cd44644b4dae703/gfx/webrender/src/renderer.rs#1557

One of the main reasons that glTexStorage2D exists is to avoid having to allocate the whole mipchain up front. I suspect fixing ANGLE to not allocate the mip-chain for glTexImage2D will not be easier than conditionally using glTexStorage2D in WebRender.

The extension is widely supported and it's relatively easy to polyfill on top of glTexImage2D so there is not much advantage to not doing it.

Comment 8

[Kelsey Gilbert [:jgilbert]]

We should detect and use glTexStorage[23]D:
https://dxr.mozilla.org/mozilla-central/rev/17c314f6930d2b8d6e456aa9e9d41407a45c3008/gfx/gl/GLContextFeatures.cpp#732

We can even polyfill glTexStorage by doing the old "don't give me mips" meme:
> glTexParameteri(target, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
> glTexParameteri(target, GL_TEXTURE_MAX_LEVEL, 0); // Requires ES3+ or any GL.
> glTexImage2D(...)

Comment 9

[Bobby Holley (:bholley)]

Seems like we ought to be able to just set GL_TEXTURE_MAX_LEVEL to zero,

Comment 10

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #9)
> Seems like we ought to be able to just set GL_TEXTURE_MAX_LEVEL to zero,

Ignore this, it was an outdated partial comment.

Per IRC discussion with kvark, gw, and jgilbert, the plan of record then is to switch to glTexStorage where available, make RT recycling only use perfect matches, and call glInvalidateFramebuffer before recycling as a bonus.

Comment 11

[Kelsey Gilbert [:jgilbert]]

I'll also mention here that we should be able to recycle same-size-or-smaller RTs, if that's useful.

Comment 12

[Bobby Holley (:bholley)]

Green try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ea7ac53c198e9048522258ae1e32ceb90d5ef97
Pending Rebased try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c56bd8f16fdca01a1a97ef59745d0b63bbb08a2

Comment 13

[Bobby Holley (:bholley)]

Attachment: Bug 1496168 - Switch FormatDesc::internal to GLenum.

That's the proper type. The fact that the glTexImage* calls take a glint
is a specific wart of that API that's not relevant to us here.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Bug 1496168 - Make creation-time immediate texture upload explicit.

The current setup doesn't work with glTexStorage. Given that it's only
used for a few callers, it's cleanest to move it to an explicit call at
those sites.

Depends on D8564

Comment 15

[Bobby Holley (:bholley)]

Attachment: Bug 1496168 - Always use a sized internal format.

This is required by glTexStorage*. We also take the opportunity to
simplify and clarify the situation with BGRA.

Depends on D8565

Comment 16

[Bobby Holley (:bholley)]

Attachment: Bug 1496168 - Use glTexStorage* where available.

Depends on D8566

Comment 17

[Bobby Holley (:bholley)]

Attachment: Bug 1496168 - Invalidate render targets when returning them to the pool.

This allows the driver to avoid preserving the old contents we don't
care about.

Depends on D8567

Comment 18

[Bobby Holley (:bholley)]

From the try results, it looks like this decreases resident memory on AWSY by ~90MB, which is great.

Comment 19

[Bobby Holley (:bholley)]

https://github.com/servo/webrender/pull/3195

Comment 20

[Bobby Holley (:bholley)]

From bug 1498650:

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #5)
> Big AWSY improvements on Windows 10 QR:
> 
> == Change summary for alert #16794 (as of Sun, 14 Oct 2018 00:20:10 GMT) ==
> 
> Improvements:
> 
>  10%  Resident Memory windows10-64-qr opt stylo     852,359,613.08 ->
> 766,955,843.66
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=16794