Bug 227226

Summary: BabylonJS Under water demo is slower than it should be on Intel
Product: WebKit Reporter: Kimmo Kinnunen <kkinnunen>
Component: WebGLAssignee: Kyle Piddington <kpiddington>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, floooh, kbr, kkinnunen, kondapallykalyan, kpiddington, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Mac (Intel)   
OS: macOS 10.15   
URL: https://playground.babylonjs.com/#LPTLZM
Bug Depends on:    
Bug Blocks: 223434, 227596    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Kimmo Kinnunen 2021-06-21 11:05:20 PDT
BabylonJS Under water demo is slower than it should be on Intel
https://playground.babylonjs.com/#LPTLZM

MacBookPro19,1 Intel / AMD 

WebGL on Metal on 10fps
WebGL on Metal off 30fps

Should be 60fps.
Comment 1 Kimmo Kinnunen 2021-06-21 11:07:18 PDT
Related to rdar://79216506 but not strictly the same
Comment 2 Radar WebKit Bug Importer 2021-06-28 11:06:17 PDT
<rdar://problem/79872484>
Comment 3 Kyle Piddington 2021-06-29 16:22:08 PDT
Created attachment 432552 [details]
Patch
Comment 4 EWS Watchlist 2021-06-29 16:23:55 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 5 Kenneth Russell 2021-06-29 16:29:33 PDT
Comment on attachment 432552 [details]
Patch

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

> Source/ThirdParty/ANGLE/ChangeLog:6
> +	Remove fastMath restriction on Intel, restore explicit [[invariant]] tag to position, fragcoord 

Please fix indentation in ChangeLog.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/EmitMetal.cpp:977
> +    (decl.isField() ? mInvariants.contains(decl.field()) : mInvariants.contains(decl.variable())) || (qualifier == TQualifier::EvqPosition || qualifier == TQualifier::EvqFragCoord);

Making gl_Position and gl_FragCoord implicitly invariant seems like a far-reaching change. If fastMath is re-enabled on Intel, do more applications than just this one need this implicit invariant qualifier on these built-in variables?
Comment 6 Kyle Piddington 2021-06-29 18:00:58 PDT
Created attachment 432563 [details]
Patch
Comment 7 Kenneth Russell 2021-06-29 21:09:08 PDT
Comment on attachment 432563 [details]
Patch

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

Looks good to me. A few minor comments about the commit message. r+

> Source/ThirdParty/ANGLE/ChangeLog:7
> +        Removing fastmath in all scenarios leads to unnacceptable performance on integrated graphics

Consider adding periods at end of sentences.

> Source/ThirdParty/ANGLE/ChangeLog:8
> +        Currently, webgl conformance tests and the safrai tests don't have any invariance tests that show issue. 

safrai -> safari

show issue -> "show issues" or "show the issue"
Comment 8 Kyle Piddington 2021-07-01 09:28:01 PDT
Created attachment 432704 [details]
Patch for landing
Comment 9 EWS 2021-07-01 10:03:25 PDT
Committed r279466 (239322@main): <https://commits.webkit.org/239322@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432704 [details].
Comment 10 Truitt Savell 2021-07-01 15:02:27 PDT
It looks like the changes in https://trac.webkit.org/changeset/279466/webkit

broke 4 Webgl tests

tracking in https://bugs.webkit.org/show_bug.cgi?id=227596
Comment 11 Kyle Piddington 2021-07-07 14:10:18 PDT
*** Bug 226841 has been marked as a duplicate of this bug. ***