Bug 198186 - [WHLSL] Standard library is too big to directly include in WebCore
Summary: [WHLSL] Standard library is too big to directly include in WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 199507 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-05-23 10:59 PDT by Myles C. Maxfield
Modified: 2019-07-05 14:15 PDT (History)
6 users (show)

See Also:


Attachments
WIP (24.02 KB, patch)
2019-06-23 23:32 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (1.27 MB, patch)
2019-06-26 13:08 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Almost full standard library (2.72 MB, patch)
2019-06-26 20:47 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Too slow (2.72 MB, patch)
2019-06-27 23:37 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Passes tests but is still too slow (2.72 MB, patch)
2019-06-28 12:51 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (2.72 MB, patch)
2019-06-28 16:48 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (2.76 MB, patch)
2019-06-29 19:52 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (2.76 MB, patch)
2019-06-29 22:12 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (2.76 MB, patch)
2019-06-29 22:31 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.71 MB, application/zip)
2019-06-30 00:30 PDT, EWS Watchlist
no flags Details
Patch (2.77 MB, patch)
2019-06-30 22:25 PDT, Myles C. Maxfield
sbarati: review+
Details | Formatted Diff | Diff
Without standard library changes (64.72 KB, patch)
2019-07-02 11:42 PDT, Myles C. Maxfield
sbarati: review+
Details | Formatted Diff | Diff
Almost ready for committing (2.78 MB, patch)
2019-07-03 09:44 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-05-23 10:59:27 PDT
The raw text of the standard library is multiple megabytes, and we can’t increase the binary of WebCore by multiple megabytes.
Comment 1 Radar WebKit Bug Importer 2019-05-30 20:34:42 PDT
<rdar://problem/51288898>
Comment 2 Saam Barati 2019-06-19 12:57:28 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
Comment 3 Myles C. Maxfield 2019-06-23 23:32:52 PDT
Created attachment 372733 [details]
WIP
Comment 4 Myles C. Maxfield 2019-06-26 13:08:59 PDT
Created attachment 372942 [details]
WIP
Comment 5 Myles C. Maxfield 2019-06-26 20:47:58 PDT
Created attachment 372998 [details]
Almost full standard library
Comment 6 Myles C. Maxfield 2019-06-26 20:48:46 PDT
Pieces missing:

step()
sign()
faceforward()
Some of the texturing functions
RWTextures
Loading from non-base mipmap
Comment 7 Myles C. Maxfield 2019-06-27 23:37:39 PDT
Created attachment 373090 [details]
Too slow
Comment 8 Myles C. Maxfield 2019-06-28 12:51:02 PDT
Created attachment 373143 [details]
Passes tests but is still too slow
Comment 9 Myles C. Maxfield 2019-06-28 16:33:48 PDT
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&)
Comment 10 Myles C. Maxfield 2019-06-28 16:42:34 PDT
29% typechecking
7% property resolver
19% building type trie in Metal codegen
14% destroying the program
5% parsing
6% preserveVariableLifetimes
Comment 11 Myles C. Maxfield 2019-06-28 16:48:42 PDT
Created attachment 373164 [details]
Patch
Comment 12 EWS Watchlist 2019-06-28 16:50:58 PDT
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.
Comment 13 Myles C. Maxfield 2019-06-29 19:52:26 PDT
Created attachment 373181 [details]
WIP
Comment 14 Myles C. Maxfield 2019-06-29 22:12:51 PDT
Created attachment 373187 [details]
Patch
Comment 15 EWS Watchlist 2019-06-29 22:14:34 PDT
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.
Comment 16 Myles C. Maxfield 2019-06-29 22:31:45 PDT
Created attachment 373188 [details]
Patch
Comment 17 EWS Watchlist 2019-06-29 22:35:40 PDT
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 18 EWS Watchlist 2019-06-30 00:30:29 PDT
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
Comment 19 EWS Watchlist 2019-06-30 00:30:31 PDT
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
Comment 20 Myles C. Maxfield 2019-06-30 22:25:40 PDT
Created attachment 373212 [details]
Patch
Comment 21 Myles C. Maxfield 2019-07-02 11:42:38 PDT
Created attachment 373343 [details]
Without standard library changes
Comment 22 Saam Barati 2019-07-02 13:02:11 PDT
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 23 Myles C. Maxfield 2019-07-02 13:15:23 PDT
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 24 Saam Barati 2019-07-02 20:59:55 PDT
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 25 Saam Barati 2019-07-02 21:00:40 PDT
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.
Comment 26 Myles C. Maxfield 2019-07-03 09:44:23 PDT
Created attachment 373392 [details]
Almost ready for committing
Comment 27 Myles C. Maxfield 2019-07-03 15:52:34 PDT
Committed r247115: <https://trac.webkit.org/changeset/247115>
Comment 28 Alexey Proskuryakov 2019-07-05 00:24:18 PDT
Making each WTF client explicitly link to libcompression doesn't seem great.
Comment 29 Ryan Haddad 2019-07-05 11:05:58 PDT
Reverted r247115 for reason:

Breaks lldbWebKitTester (and by extension, test-webkitpy)

Committed r247164: <https://trac.webkit.org/changeset/247164>
Comment 30 Ryan Haddad 2019-07-05 11:07:07 PDT
*** Bug 199507 has been marked as a duplicate of this bug. ***
Comment 31 Myles C. Maxfield 2019-07-05 14:15:08 PDT
Committed r247174: <https://trac.webkit.org/changeset/247174>