Bug 234008 – ANGLE Metal framebuffer object clear performance regression

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Jekfer Bichon]

Created attachment 446365 [details]
Sample to reproduce

Hello,

We have detected a performance regression while manipulating framebuffers. I have attached a sample where I bind and clear framebuffers that are created once. The regression has been detected on an iPad Gen 8.

We were getting a comfortable 60 fps on the sample on Safari 14 while we are barely hitting 21 fps on Safari 15. Disabling the experimental option "WebGL via Metal" on Safari 15 puts the performances back at Safari 14 level.

Thank you.

[Radar WebKit Bug Importer]

<rdar://problem/86522988>

[Kimmo Kinnunen]

I can reproduce slight regressions in perf of iPhone 11, iPad Pro 2015, nothing as drastic as reported. iPhone 11 fps == ~30, iPad Pro 2015 fps == ~10

All samples under 
Sample Count, Samples %, Normalized CPU %, Symbol
209, 4.4%, 3.2%, WebCore::GraphicsContextGLANGLE::clear(unsigned int) (in WebCore)
209, 4.4%, 3.2%,     gl::Clear(unsigned int) (in libANGLE-shared.dylib)
173, 3.6%, 2.6%,         gl::Context::clear(unsigned int) (in libANGLE-shared.dylib)
169, 3.5%, 2.6%,             gl::Context::syncState(angle::IterableBitSet<63ul> const&, angle::BitSetT<12ul, unsigned int, unsigned long> const&, gl::Command) (in libANGLE-shared.dylib)
166, 3.5%, 2.5%,                 rx::ContextMtl::syncState(gl::Context const*, angle::IterableBitSet<63ul> const&, angle::IterableBitSet<63ul> const&) (in libANGLE-shared.dylib)
165, 3.5%, 2.5%,                     rx::ContextMtl::onDrawFrameBufferChangedState(gl::Context const*, rx::FramebufferMtl*, bool) (in libANGLE-shared.dylib)
165, 3.5%, 2.5%,                         rx::ContextMtl::endEncoding(bool) (in libANGLE-shared.dylib)
165, 3.5%, 2.5%,                             rx::ContextMtl::endRenderEncoding(rx::mtl::RenderCommandEncoder*) (in libANGLE-shared.dylib)
163, 3.4%, 2.5%,                                 rx::mtl::RenderCommandEncoder::endEncodingImpl(bool) (in libANGLE-shared.dylib)

[Vincent Meynier]

Hello, at Dassault Systèmes, all our 3D web applications are impacted by this regression.
"CATIA xGenerative Design" and "SolidWorks xDesign" are particularly concerned.
Our Apple users unfortunately can no longer take advantage of an essential feature, because less than 30 FPS was not acceptable, so we have restricted the use while waiting for a solution.

Kimmo, I'm sorry I'm not very familiar with bugs.webkit.org
- As a Reviewer, you validated the relevance of the issue and confirmed that the sample works, and now a developer needs to take the ticket? 
- What chance do we have that this one will be dealt with soon?
- How long after the fix will it take before our users' browsers are updated with it?

Thank you very much for your time and your help.

[Sébastien VIDECOQ]

Hello, any progress on this ?

[Kimmo Kinnunen]

I can still reproduce this on iOS 15.4 seed 4, iPad Pro 2020, portrait:
WebGL via OpenGL 50fps (onscreen counter)
WebGL via Metal 26fps (onscreen counter)

Will investigate this further.

[Dan Glastonbury]

Created attachment 456665 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Dan Glastonbury]

Created attachment 456673 [details]
Patch

[Dan Glastonbury]

Comment on attachment 456673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456673&action=review

> Source/ThirdParty/ANGLE/changes.diff:-19425
> -diff --git a/src/compiler/translator/TranslatorMetalDirect.cpp b/src/compiler/translator/TranslatorMetalDirect.cpp

I didn't make these changes

> Source/ThirdParty/ANGLE/changes.diff:-19840
> -diff --git a/src/libANGLE/renderer/metal/FrameBufferMtl.mm b/src/libANGLE/renderer/metal/FrameBufferMtl.mm

I didn't make these changes

> Source/ThirdParty/ANGLE/changes.diff:-19931
> -@@ -775,5 +775,4 @@ void PBufferSurfaceMtl::setFixedHeight(EGLint height)

I didn't make these changes

[Kimmo Kinnunen]

Comment on attachment 456673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456673&action=review

some comments about  perhaps need for clarifications

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.h:579
> +    uint32_t mRenderPassCount = 0;

The variable name should be more descriptive. It does not explain that it counts the number of render passes since last flush.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:1704
> +        mRenderPassCount = 0;

I don't think it's correct to reset the render pass count only when the render pass count exceeds 16.
You need to reset it somehow else, at some other point too. E.g. for every flush?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:1705
> +        flushCommandBuffer(mtl::WaitUntilScheduled);

Is there a reason:
-  exceeding max render passes needs WaitUntilScheduled
vs 
- exceeding max call count needs NoWait

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_command_buffer.h:171
> +    uint32_t mRenderPasses      = 0;

is this variable used at all?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_common.h:151
> +constexpr uint32_t kMaxRenderPasses = 16;

kMaxRenderPasses per some quality (in-flight what? You might have 1000 command buffers "in flight", does it mean you can only have 16 render passes total "in flight"?)

kMaxRenderPassesPerCommandBuffer ?

[Kimmo Kinnunen]

For the commit message, you might spend a sentence or two about what it is that makes having more than 16 render passes in a command buffer slow?

[Dan Glastonbury]

Created attachment 456783 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 456783 [details]
Patch

nice find!

[Kenneth Russell]

Great find and fix! It would be ideal to upstream this to ANGLE as soon as possible.

[Dan Glastonbury]

Pull request: https://github.com/WebKit/WebKit/pull/1201

[EWS]

Committed r295082 (251177@main): <https://commits.webkit.org/251177@main>

Reviewed commits have been landed. Closing PR #1201 and removing active labels.