| Summary: | [WHLSL] Standard library is too big to directly include in WebCore | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||
| Component: | WebGPU | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | aakash_jain, ews-watchlist, jonlee, ryanhaddad, saam, webkit-bug-importer | ||||||||||||||||||||||||||||
| Priority: | P1 | Keywords: | InRadar | ||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=192890 | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Myles C. Maxfield
2019-05-23 10:59:27 PDT
Ideas Myles has suggested: - Gzip the stdlib text into the binary - To combat large compile times, prune the call graph - Remove some stdlib from text, and build it in as some kind of an intrinsic in the compiler itself Some of these tasks should be independent of each other Created attachment 372733 [details]
WIP
Created attachment 372942 [details]
WIP
Created attachment 372998 [details]
Almost full standard library
Pieces missing: step() sign() faceforward() Some of the texturing functions RWTextures Loading from non-base mipmap Created attachment 373090 [details]
Too slow
Created attachment 373143 [details]
Passes tests but is still too slow
4.07 s 98.2% 0 s WebCore::WHLSL::prepare(WTF::String&, WebCore::WHLSL::ComputePipelineDescriptor&) 2.46 s 59.3% 1.00 ms WebCore::WHLSL::prepareShared(WTF::String&) 1.20 s 28.8% 0 s WebCore::WHLSL::check(WebCore::WHLSL::Program&) 389.00 ms 9.3% 0 s WebCore::WHLSL::Visitor::visit(WebCore::WHLSL::Program&) 286.00 ms 6.9% 0 s WebCore::WHLSL::PropertyResolver::visit(WebCore::WHLSL::AST::FunctionDefinition&) 67.00 ms 1.6% 0 s WebCore::WHLSL::RecursionChecker::visit(WebCore::WHLSL::AST::FunctionDefinition&) 22.00 ms 0.5% 0 s WebCore::WHLSL::Visitor::visit(WebCore::WHLSL::AST::Block&) 11.00 ms 0.2% 0 s WebCore::WHLSL::Visitor::visit(WebCore::WHLSL::AST::FunctionDeclaration&) 3.00 ms 0.0% 0 s WebCore::WHLSL::Visitor::visit(WebCore::WHLSL::AST::FunctionDefinition&) 249.00 ms 6.0% 0 s WebCore::WHLSL::preserveVariableLifetimes(WebCore::WHLSL::Program&) 226.00 ms 5.4% 0 s WebCore::WHLSL::Parser::parse(WebCore::WHLSL::Program&, WTF::StringView, WebCore::WHLSL::Parser::Mode) 143.00 ms 3.4% 1.00 ms WebCore::WHLSL::NameResolver::visit(WebCore::WHLSL::AST::FunctionDefinition&) 83.00 ms 2.0% 0 s WebCore::WHLSL::autoInitializeVariables(WebCore::WHLSL::Program&) 64.00 ms 1.5% 0 s WebCore::WHLSL::checkTextureReferences(WebCore::WHLSL::Program&) 44.00 ms 1.0% 0 s WebCore::WHLSL::NameResolver::visit(WebCore::WHLSL::AST::NativeFunctionDeclaration&) 25.00 ms 0.6% 0 s WebCore::WHLSL::synthesizeConstructors(WebCore::WHLSL::Program&) 23.00 ms 0.5% 0 s WebCore::WHLSL::synthesizeArrayOperatorLength(WebCore::WHLSL::Program&) 8.00 ms 0.1% 0 s WebCore::WHLSL::checkDuplicateFunctions(WebCore::WHLSL::Program const&) 7.00 ms 0.1% 0 s WebCore::WHLSL::checkStatementBehavior(WebCore::WHLSL::Program&) 1.00 ms 0.0% 1.00 ms WebCore::WHLSL::checkFunctionStages(WebCore::WHLSL::Program&) 1.00 ms 0.0% 0 s WebCore::WHLSL::resolveNamesInTypes(WebCore::WHLSL::Program&, WebCore::WHLSL::NameResolver&) 927.00 ms 22.3% 0 s WebCore::WHLSL::Metal::generateMetalCode(WebCore::WHLSL::Program&, WebCore::WHLSL::MatchedComputeSemantics&&, WTF::Vector<WebCore::WHLSL::BindGroup, 0ul, WTF::CrashOnOverflow, 16ul>&) 923.00 ms 22.2% 0 s WebCore::WHLSL::Metal::TypeNamer::metalTypes() 826.00 ms 19.9% 0 s WebCore::WHLSL::Visitor::visit(WebCore::WHLSL::Program&) 94.00 ms 2.2% 0 s WebCore::WHLSL::Metal::TypeNamer::metalTypeDeclarations() 3.00 ms 0.0% 0 s WebCore::WHLSL::Metal::TypeNamer::metalTypeDefinitions() 4.00 ms 0.0% 1.00 ms WebCore::WHLSL::Metal::metalFunctions(WebCore::WHLSL::Program&, WebCore::WHLSL::Metal::TypeNamer&, WebCore::WHLSL::MatchedComputeSemantics&&, WTF::Vector<WebCore::WHLSL::BindGroup, 0ul, WTF::CrashOnOverflow, 16ul>&) 599.00 ms 14.4% 3.00 ms WebCore::WHLSL::Program::~Program() 87.00 ms 2.0% 0 s WebCore::WHLSL::computeDimensions(WebCore::WHLSL::Program&, WebCore::WHLSL::AST::FunctionDefinition&) 29% typechecking 7% property resolver 19% building type trie in Metal codegen 14% destroying the program 5% parsing 6% preserveVariableLifetimes Created attachment 373164 [details]
Patch
Attachment 373164 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 373181 [details]
WIP
Created attachment 373187 [details]
Patch
Attachment 373187 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 373188 [details]
Patch
Attachment 373188 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 22 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 373188 [details] Patch Attachment 373188 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12617205 New failing tests: http/tests/cache/disk-cache/redirect-chain-limits.html Created attachment 373189 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5
Created attachment 373212 [details]
Patch
Created attachment 373343 [details]
Without standard library changes
Comment on attachment 373343 [details] Without standard library changes View in context: https://bugs.webkit.org/attachment.cgi?id=373343&action=review r=me > Source/WebCore/ChangeLog:29 > + form /* Functions named xyz */. At build time, a Python script looks for all these comments, and builds a Is this being done in the spec repo? > Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:44 > +static Vector<LChar> decompressStandardLibrary() Do we not just have a helper "gunzip" method in WebKit? If not, maybe we can add a "gunzip" helper in WTF? > Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:112 > + HashSet<String> result; > + std::swap(result, m_functionNames); > + return result; style nit: Why not: "return WTFMove(m_functionNames)"? > Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:151 > + functionNames.add("operator cast"_str); > + functionNames.add("ddx"_str); > + functionNames.add("ddy"_str); > + functionNames.add("DeviceMemoryBarrierWithGroupSync"_str); > + functionNames.add("GroupMemoryBarrierWithGroupSync"_str); > + functionNames.add("AllMemoryBarrierWithGroupSync"_str); This means we always parse these? Why is that needed? Maybe a comment? > Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:166 > + functionNames.clear(); style nit: this isn't necessary since you're assigning to it below. > Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:177 > +} > + > +} style nit: add namespace ending comments > Source/WebCore/Modules/webgpu/WHLSL/WHLSLBuildStandardLibraryFunctionMap.py:87 > + outfile.write(" result.add(\"" + previousName + "\"_str, SubstringLocation { " + str(previous) + ", " + str(match.start()) + " });\n") > + previous = match.start() > + previousName = match.group(1) > +outfile.write(" result.add(\"" + previousName + "\"_str, SubstringLocation { " + str(previous) + ", " + str(len(contents)) + " });\n") Can you open a bug and add a FIXME here about just computing the hash table at compile time in the future? Kinda like we do with the static property hash tables in JSC. We could probably just reuse that code. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:420 > + || functionDefinition.name() == "operator>>") 👍🏼 > LayoutTests/webgpu/whlsl-matrix.html:39 > + foo = foo + float2x3(float3(1.0, 1.0, 1.0), float3(1.0, 1.0, 1.0)); > if (!allEqual(foo, 1)) > return; > > - foo = foo * 8.5; > + foo = foo * float2x3(float3(8.5, 8.5, 8.5), float3(8.5, 8.5, 8.5)); > if (!allEqual(foo, 8.5)) > return; > > - foo = foo - 7.5; > + foo = foo - float2x3(float3(7.5, 7.5, 7.5), float3(7.5, 7.5, 7.5)); > if (!allEqual(foo, 1)) > return; > > - foo = foo + 1.0; > + foo = foo + float2x3(float3(1.0, 1.0, 1.0), float3(1.0, 1.0, 1.0)); Why? Isn't this part of the stdlib to do math with a single parameter against all matrix elements? Comment on attachment 373343 [details] Without standard library changes View in context: https://bugs.webkit.org/attachment.cgi?id=373343&action=review >> Source/WebCore/ChangeLog:29 >> + form /* Functions named xyz */. At build time, a Python script looks for all these comments, and builds a > > Is this being done in the spec repo? Nope. I think it's okay if we don't impose the rigorous structure we require in WebKit on the GitHub spec repository. >> Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:112 >> + return result; > > style nit: > Why not: "return WTFMove(m_functionNames)"? Even after running this function, m_functionNames needs to be valid (and empty). If we just moved from it, we would then be using a moved-from object, which is undefined. >> LayoutTests/webgpu/whlsl-matrix.html:39 >> + foo = foo + float2x3(float3(1.0, 1.0, 1.0), float3(1.0, 1.0, 1.0)); > > Why? Isn't this part of the stdlib to do math with a single parameter against all matrix elements? Nope! We should add it to the spec repo first, though. Comment on attachment 373343 [details] Without standard library changes View in context: https://bugs.webkit.org/attachment.cgi?id=373343&action=review >>> Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:112 >>> + return result; >> >> style nit: >> Why not: "return WTFMove(m_functionNames)"? > > Even after running this function, m_functionNames needs to be valid (and empty). If we just moved from it, we would then be using a moved-from object, which is undefined. FYI, that's not quite right. Moving from things defined in the C++ stdlib is undefined. Moving from things where move operator is defined is not undefined. Comment on attachment 373343 [details] Without standard library changes View in context: https://bugs.webkit.org/attachment.cgi?id=373343&action=review >>>> Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:112 >>>> + return result; >>> >>> style nit: >>> Why not: "return WTFMove(m_functionNames)"? >> >> Even after running this function, m_functionNames needs to be valid (and empty). If we just moved from it, we would then be using a moved-from object, which is undefined. > > FYI, that's not quite right. Moving from things defined in the C++ stdlib is undefined. Moving from things where move operator is defined is not undefined. Anyways, if it wasn't obvious, I think it's ok to keep it as you have it defined now. Created attachment 373392 [details]
Almost ready for committing
Committed r247115: <https://trac.webkit.org/changeset/247115> Making each WTF client explicitly link to libcompression doesn't seem great. Reverted r247115 for reason: Breaks lldbWebKitTester (and by extension, test-webkitpy) Committed r247164: <https://trac.webkit.org/changeset/247164> *** Bug 199507 has been marked as a duplicate of this bug. *** Committed r247174: <https://trac.webkit.org/changeset/247174> |