Closed
Bug 1496168
Opened 6 years ago
Closed 6 years ago
ANGLE generates mipmaps for WebRender textures unconditionally
Categories
(Core :: Graphics: WebRender, enhancement, P2)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
People
(Reporter: bholley, Assigned: bholley)
References
(Regressed 1 open bug)
Details
(Whiteboard: [MemShrink:P1])
Attachments
(5 files)
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
Assignee | ||
Comment 1•6 years ago
|
||
Jeff, am I missing something here? If not, what shall we do about it?
Flags: needinfo?(jgilbert)
Assignee | ||
Updated•6 years ago
|
Summary: ANGLE generates mipmaps for WebRender textures without asking → ANGLE generates mipmaps for WebRender textures unconditionally
Comment 2•6 years ago
|
||
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•6 years ago
|
||
Isn't TexStorage2D only available in GL4+ though? (although perhaps the extension version is widely available in GL3?)
Assignee | ||
Comment 4•6 years ago
|
||
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•6 years ago
|
||
(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.
Assignee | ||
Comment 6•6 years ago
|
||
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•6 years ago
|
||
(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•6 years ago
|
||
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(...)
Flags: needinfo?(jgilbert)
Assignee | ||
Comment 9•6 years ago
|
||
Seems like we ought to be able to just set GL_TEXTURE_MAX_LEVEL to zero,
Assignee: nobody → bobbyholley
Assignee | ||
Comment 10•6 years ago
|
||
(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•6 years ago
|
||
I'll also mention here that we should be able to recycle same-size-or-smaller RTs, if that's useful.
Updated•6 years ago
|
Priority: -- → P2
Updated•6 years ago
|
Blocks: stage-wr-trains
Assignee | ||
Comment 12•6 years ago
|
||
Assignee | ||
Comment 13•6 years ago
|
||
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.
Assignee | ||
Comment 14•6 years ago
|
||
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
Assignee | ||
Comment 15•6 years ago
|
||
This is required by glTexStorage*. We also take the opportunity to
simplify and clarify the situation with BGRA.
Depends on D8565
Assignee | ||
Comment 16•6 years ago
|
||
Depends on D8566
Assignee | ||
Comment 17•6 years ago
|
||
This allows the driver to avoid preserving the old contents we don't
care about.
Depends on D8567
Assignee | ||
Comment 18•6 years ago
|
||
From the try results, it looks like this decreases resident memory on AWSY by ~90MB, which is great.
Blocks: 1495915
Assignee | ||
Comment 19•6 years ago
|
||
Updated•6 years ago
|
Depends on: 1498650
See Also: → https://github.com/servo/webrender/pull/3195
Assignee | ||
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 20•6 years ago
|
||
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
Updated•6 years ago
|
Whiteboard: [MemShrink:P1]
You need to log in
before you can comment on or make changes to this bug.
Description
•