Bug 188943 - [WebAssembly] Parse wasm modules in a streaming fashion
Summary: [WebAssembly] Parse wasm modules in a streaming fashion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebAssembly (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-25 04:49 PDT by Yusuke Suzuki
Modified: 2018-08-27 23:39 PDT (History)
10 users (show)

See Also:


Attachments
Patch (116.91 KB, patch)
2018-08-26 14:54 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (163.13 KB, patch)
2018-08-27 06:11 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.10 KB, patch)
2018-08-27 07:12 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.10 KB, patch)
2018-08-27 07:13 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.37 KB, patch)
2018-08-27 07:16 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.30 KB, patch)
2018-08-27 07:20 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.30 KB, patch)
2018-08-27 08:26 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.46 KB, patch)
2018-08-27 09:05 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.47 KB, patch)
2018-08-27 09:07 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (167.43 KB, patch)
2018-08-27 11:09 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-08-25 04:49:06 PDT
Enable stream decoding!
Comment 1 Yusuke Suzuki 2018-08-26 14:54:45 PDT
Created attachment 348102 [details]
Patch
Comment 2 Keith Miller 2018-08-27 01:42:04 PDT
Did you mean to mark this for review?
Comment 3 Yusuke Suzuki 2018-08-27 01:49:44 PDT
(In reply to Keith Miller from comment #2)
> Did you mean to mark this for review?

Not yet :) I'll do some cleaning up and set r=? later!
Currently my plan is the following,

1. In this patch, we separate SectionParser from ModuleParser, use SectionParser in old and new wasm parsers. Adding very basic tests specifically for our streaming parser, but we do not integrate streaming parser to our wasm pipeline yet in this patch.
2. In the subsequent patch, we change our BBQPlan to kick streaming parser, and streaming compilation.
Comment 4 Yusuke Suzuki 2018-08-27 06:11:26 PDT
Created attachment 348136 [details]
Patch
Comment 5 Yusuke Suzuki 2018-08-27 06:11:58 PDT
(In reply to Yusuke Suzuki from comment #1)
> Created attachment 348102 [details]
> Patch

Ready for reviews!
Comment 6 EWS Watchlist 2018-08-27 06:14:03 PDT
Attachment 348136 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmSectionParser.h:58:  Do not use 'using namespace FailureHelper;'.  [build/using_namespace] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:211:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:377:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Yusuke Suzuki 2018-08-27 07:12:08 PDT
Created attachment 348139 [details]
Patch
Comment 8 Yusuke Suzuki 2018-08-27 07:13:37 PDT
Created attachment 348140 [details]
Patch
Comment 9 EWS Watchlist 2018-08-27 07:14:55 PDT
Attachment 348140 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmSectionParser.h:58:  Do not use 'using namespace FailureHelper;'.  [build/using_namespace] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:211:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:377:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Yusuke Suzuki 2018-08-27 07:16:40 PDT
Created attachment 348141 [details]
Patch
Comment 11 EWS Watchlist 2018-08-27 07:17:58 PDT
Attachment 348141 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmSectionParser.h:58:  Do not use 'using namespace FailureHelper;'.  [build/using_namespace] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:211:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:377:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Yusuke Suzuki 2018-08-27 07:20:45 PDT
Created attachment 348142 [details]
Patch
Comment 13 EWS Watchlist 2018-08-27 07:23:29 PDT
Attachment 348142 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmSectionParser.h:58:  Do not use 'using namespace FailureHelper;'.  [build/using_namespace] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:210:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:376:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Yusuke Suzuki 2018-08-27 08:26:13 PDT
Created attachment 348144 [details]
Patch
Comment 15 EWS Watchlist 2018-08-27 08:29:45 PDT
Attachment 348144 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmSectionParser.h:58:  Do not use 'using namespace FailureHelper;'.  [build/using_namespace] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:210:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:376:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Yusuke Suzuki 2018-08-27 08:49:38 PDT
Comment on attachment 348144 [details]
Patch

OK, ready.
Comment 17 Yusuke Suzuki 2018-08-27 09:05:44 PDT
Created attachment 348146 [details]
Patch
Comment 18 Yusuke Suzuki 2018-08-27 09:07:13 PDT
Created attachment 348147 [details]
Patch
Comment 19 EWS Watchlist 2018-08-27 09:11:51 PDT
Attachment 348147 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmSectionParser.h:58:  Do not use 'using namespace FailureHelper;'.  [build/using_namespace] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:210:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Yusuke Suzuki 2018-08-27 11:09:08 PDT
Created attachment 348167 [details]
Patch
Comment 21 EWS Watchlist 2018-08-27 11:10:43 PDT
Attachment 348167 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmSectionParser.h:58:  Do not use 'using namespace FailureHelper;'.  [build/using_namespace] [4]
ERROR: Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:210:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Mark Lam 2018-08-27 15:31:23 PDT
Comment on attachment 348167 [details]
Patch

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

r=me with some suggested improvements.

> Source/JavaScriptCore/ChangeLog:25
> +        This object is introduced for testing purpose. Added new stress test uses this interface
> +        to test streaming parser in the JSC shell.

I suggest you make it explicit that "This object" means "The $vm WasmStreamingParser object".

> Source/JavaScriptCore/ChangeLog:51
> +        memoryCount and tableCount should be recoreded in ModuleInformation.

/recoreded/recorded/.

> Source/JavaScriptCore/ChangeLog:106
> +        SectionParser is extracted from ModuleParser. And it is used both the old (currently working) ModuleParser

/used both/used by both/.

> Source/JavaScriptCore/ChangeLog:136
> +        bunch of functions. StreamingParser extracts each function in a streaming fashion and enable streaming

/bunch of/a bunch of/.

> Source/JavaScriptCore/ChangeLog:153
> +        the user calls StreamingParser::finalize. StreamingParser is a state machine which feeds the incoming

/feeds the/feeds on the/.

> Source/JavaScriptCore/wasm/WasmModuleInformation.h:65
> +    uint32_t memoryCount() const { return memory ? 1 : 0; }
> +    uint32_t tableCount() const { return tableInformation ? 1 : 0; }

How does this work?  It seems we're always returning 1 for the counts whereas the old code increments m_tableCount in ModuleParser.  I don't see tableCount being tracked anymore.

I see in SectionParser::parseExport() that we're checking "kindIndex >= m_info->tableCount()".  This suggests to me that that check is not functioning as intended, but your implementation of tableCount() is effectively just defeating the check.
Ditto for SectionParser::parseElement().

Oh, I see from SectionParser::parseTable() that we're allowed only 1 table.  I presume this is WASM spec thing.  Can you please add a comment here to explain that?  It took me a while to tease out this reason for why "tableInformation ? 1 : 0" is valid.  I suspect other folks who don't already intimately know the spec may find this equally confusing.

Ditto for memoryCount.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:218
> +    WASM_PARSER_FAIL_IF(count > 1, "Table count of ", count, " is invalid, at most 1 is allowed for now");

Ohh ... we're only ever allowed 1 table.  That explains why we don't update a tableCount anymore.

Please add a comment in ModuleInformation::tableCount() about this constraint.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:268
> +    WASM_PARSER_FAIL_IF(count != 1, "Memory section has more than one memory, WebAssembly currently only allows zero or one");

Ditto: please add a comment in ModuleInformation::memoryCount() about this constraint.

> Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:250
> +    if (Options::useEagerWebAssemblyModuleHashing())

UNLIKELY?

> Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:255
> +    while (true) {
> +        switch (m_state) {

Please ASSERT(offsetInBytes <= bytesSize) above this switch.

> Source/JavaScriptCore/wasm/WasmStreamingParser.h:55
> +    // So this streaming parser handles Section as an unit for incremental parsing. The exception is Code section. The code section is large since it

/an unit/the unit/.
/is Code section/is the Code section/.

> Source/JavaScriptCore/wasm/WasmStreamingParser.h:57
> +    // parser specially handles the code section. In the code section, the streaming parser uses Function as an unit for incremental parsing.

/an unit/the unit/.
Comment 23 Yusuke Suzuki 2018-08-27 23:33:52 PDT
Comment on attachment 348167 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/ChangeLog:25
>> +        to test streaming parser in the JSC shell.
> 
> I suggest you make it explicit that "This object" means "The $vm WasmStreamingParser object".

Fixed.

>> Source/JavaScriptCore/ChangeLog:51
>> +        memoryCount and tableCount should be recoreded in ModuleInformation.
> 
> /recoreded/recorded/.

Fixed.

>> Source/JavaScriptCore/ChangeLog:106
>> +        SectionParser is extracted from ModuleParser. And it is used both the old (currently working) ModuleParser
> 
> /used both/used by both/.

Fixed.

>> Source/JavaScriptCore/ChangeLog:136
>> +        bunch of functions. StreamingParser extracts each function in a streaming fashion and enable streaming
> 
> /bunch of/a bunch of/.

Fixed.

>> Source/JavaScriptCore/ChangeLog:153
>> +        the user calls StreamingParser::finalize. StreamingParser is a state machine which feeds the incoming
> 
> /feeds the/feeds on the/.

Fixed.

>> Source/JavaScriptCore/wasm/WasmModuleInformation.h:65
>> +    uint32_t tableCount() const { return tableInformation ? 1 : 0; }
> 
> How does this work?  It seems we're always returning 1 for the counts whereas the old code increments m_tableCount in ModuleParser.  I don't see tableCount being tracked anymore.
> 
> I see in SectionParser::parseExport() that we're checking "kindIndex >= m_info->tableCount()".  This suggests to me that that check is not functioning as intended, but your implementation of tableCount() is effectively just defeating the check.
> Ditto for SectionParser::parseElement().
> 
> Oh, I see from SectionParser::parseTable() that we're allowed only 1 table.  I presume this is WASM spec thing.  Can you please add a comment here to explain that?  It took me a while to tease out this reason for why "tableInformation ? 1 : 0" is valid.  I suspect other folks who don't already intimately know the spec may find this equally confusing.
> 
> Ditto for memoryCount.

OK, I added the comment.

>> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:218
>> +    WASM_PARSER_FAIL_IF(count > 1, "Table count of ", count, " is invalid, at most 1 is allowed for now");
> 
> Ohh ... we're only ever allowed 1 table.  That explains why we don't update a tableCount anymore.
> 
> Please add a comment in ModuleInformation::tableCount() about this constraint.

OK, added.

>> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:268
>> +    WASM_PARSER_FAIL_IF(count != 1, "Memory section has more than one memory, WebAssembly currently only allows zero or one");
> 
> Ditto: please add a comment in ModuleInformation::memoryCount() about this constraint.

Yeah, added.

>> Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:250
>> +    if (Options::useEagerWebAssemblyModuleHashing())
> 
> UNLIKELY?

Added.

>> Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:255
>> +        switch (m_state) {
> 
> Please ASSERT(offsetInBytes <= bytesSize) above this switch.

Added.

>> Source/JavaScriptCore/wasm/WasmStreamingParser.h:55
>> +    // So this streaming parser handles Section as an unit for incremental parsing. The exception is Code section. The code section is large since it
> 
> /an unit/the unit/.
> /is Code section/is the Code section/.

Fixed.

>> Source/JavaScriptCore/wasm/WasmStreamingParser.h:57
>> +    // parser specially handles the code section. In the code section, the streaming parser uses Function as an unit for incremental parsing.
> 
> /an unit/the unit/.

Fixed.
Comment 24 Yusuke Suzuki 2018-08-27 23:34:02 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=348167&action=review

Thank you!

>> Source/JavaScriptCore/ChangeLog:25
>> +        to test streaming parser in the JSC shell.
> 
> I suggest you make it explicit that "This object" means "The $vm WasmStreamingParser object".

Fixed.

>> Source/JavaScriptCore/ChangeLog:51
>> +        memoryCount and tableCount should be recoreded in ModuleInformation.
> 
> /recoreded/recorded/.

Fixed.

>> Source/JavaScriptCore/ChangeLog:106
>> +        SectionParser is extracted from ModuleParser. And it is used both the old (currently working) ModuleParser
> 
> /used both/used by both/.

Fixed.

>> Source/JavaScriptCore/ChangeLog:136
>> +        bunch of functions. StreamingParser extracts each function in a streaming fashion and enable streaming
> 
> /bunch of/a bunch of/.

Fixed.

>> Source/JavaScriptCore/ChangeLog:153
>> +        the user calls StreamingParser::finalize. StreamingParser is a state machine which feeds the incoming
> 
> /feeds the/feeds on the/.

Fixed.

>> Source/JavaScriptCore/wasm/WasmModuleInformation.h:65
>> +    uint32_t tableCount() const { return tableInformation ? 1 : 0; }
> 
> How does this work?  It seems we're always returning 1 for the counts whereas the old code increments m_tableCount in ModuleParser.  I don't see tableCount being tracked anymore.
> 
> I see in SectionParser::parseExport() that we're checking "kindIndex >= m_info->tableCount()".  This suggests to me that that check is not functioning as intended, but your implementation of tableCount() is effectively just defeating the check.
> Ditto for SectionParser::parseElement().
> 
> Oh, I see from SectionParser::parseTable() that we're allowed only 1 table.  I presume this is WASM spec thing.  Can you please add a comment here to explain that?  It took me a while to tease out this reason for why "tableInformation ? 1 : 0" is valid.  I suspect other folks who don't already intimately know the spec may find this equally confusing.
> 
> Ditto for memoryCount.

OK, I added the comment.

>> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:218
>> +    WASM_PARSER_FAIL_IF(count > 1, "Table count of ", count, " is invalid, at most 1 is allowed for now");
> 
> Ohh ... we're only ever allowed 1 table.  That explains why we don't update a tableCount anymore.
> 
> Please add a comment in ModuleInformation::tableCount() about this constraint.

OK, added.

>> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:268
>> +    WASM_PARSER_FAIL_IF(count != 1, "Memory section has more than one memory, WebAssembly currently only allows zero or one");
> 
> Ditto: please add a comment in ModuleInformation::memoryCount() about this constraint.

Yeah, added.

>> Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:250
>> +    if (Options::useEagerWebAssemblyModuleHashing())
> 
> UNLIKELY?

Added.

>> Source/JavaScriptCore/wasm/WasmStreamingParser.cpp:255
>> +        switch (m_state) {
> 
> Please ASSERT(offsetInBytes <= bytesSize) above this switch.

Added.

>> Source/JavaScriptCore/wasm/WasmStreamingParser.h:55
>> +    // So this streaming parser handles Section as an unit for incremental parsing. The exception is Code section. The code section is large since it
> 
> /an unit/the unit/.
> /is Code section/is the Code section/.

Fixed.

>> Source/JavaScriptCore/wasm/WasmStreamingParser.h:57
>> +    // parser specially handles the code section. In the code section, the streaming parser uses Function as an unit for incremental parsing.
> 
> /an unit/the unit/.

Fixed.
Comment 25 Yusuke Suzuki 2018-08-27 23:38:35 PDT
Committed r235420: <https://trac.webkit.org/changeset/235420>
Comment 26 Radar WebKit Bug Importer 2018-08-27 23:39:17 PDT
<rdar://problem/43786871>