Bug 224991

Summary: [Metal ANGLE] Temporarily remove AST validation even for debug builds
Product: WebKit Reporter: John Cunningham <johncunningham>
Component: WebGLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, graouts, kbr, kkinnunen, kondapallykalyan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

John Cunningham
Reported 2021-04-23 13:25:24 PDT
[Metal ANGLE] Temporarily remove AST validation even for debug builds
Attachments
Patch (1.38 KB, patch)
2021-04-23 13:26 PDT, John Cunningham 		no flags
DetailsFormatted DiffDiff
Patch (1.55 KB, patch)
2021-04-23 14:15 PDT, John Cunningham 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
John Cunningham
Comment 1 2021-04-23 13:26:34 PDT
Created attachment 426937 [details] Patch
John Cunningham
Comment 2 2021-04-23 13:26:37 PDT
<rdar://problem/76299178>
EWS Watchlist
Comment 3 2021-04-23 13:27:22 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Kenneth Russell
Comment 4 2021-04-23 13:59:43 PDT
Could you please provide a description in this bug of why this is being done, and how temporarily this is being removed?
John Cunningham
Comment 5 2021-04-23 14:08:36 PDT
Sure thing. We are seeing significant slow downs inside of the validateAST calls on debug builds leading to test timeouts. There are a significant number of transform passes as a part of the GLSL AST -> MSL translation, and currently each pass is validating the AST. Theses were mainly in place during development of the translator, and the correct fix is to move the validation to the very end after all of the transformations happen. To unblock further testing, we'd like to land this stopgap to run all of the debug testing, until we land the more correct fix (hopefully within the next few days.)
Kenneth Russell
Comment 6 2021-04-23 14:09:44 PDT
Comment on attachment 426937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426937&action=review Thanks for the explanation. r+ > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ShaderMtl.mm:88 > +#if defined(ANGLE_ENABLE_ASSERTS) && 0 Please add some TODO here regarding moving this validation in the future so we don't forget it.
John Cunningham
Comment 7 2021-04-23 14:15:32 PDT
Created attachment 426942 [details] Patch
Kenneth Russell
Comment 8 2021-04-23 14:16:31 PDT
Comment on attachment 426942 [details] Patch Thanks, still looks good. r+ again
EWS
Comment 9 2021-04-25 12:16:33 PDT
Committed r276568 (237004@main): <https://commits.webkit.org/237004@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426942 [details].
Note You need to log in before you can comment on or make changes to this bug.