Bug 188943 – [WebAssembly] Parse wasm modules in a streaming fashion

• Status: RESOLVED
• Resolution: FIXED
• Component: WebAssembly

[Yusuke Suzuki]

Enable stream decoding!

[Yusuke Suzuki]

Created attachment 348102 [details]
Patch

[Keith Miller]

Did you mean to mark this for review?

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 348136 [details]
Patch

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #1)
> Created attachment 348102 [details]
> Patch

Ready for reviews!

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 348139 [details]
Patch

[Yusuke Suzuki]

Created attachment 348140 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 348141 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 348142 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 348144 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Comment on attachment 348144 [details]
Patch

OK, ready.

[Yusuke Suzuki]

Created attachment 348146 [details]
Patch

[Yusuke Suzuki]

Created attachment 348147 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 348167 [details]
Patch

[EWS Watchlist]

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.

[Mark Lam]

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/.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r235420: <https://trac.webkit.org/changeset/235420>

[Radar WebKit Bug Importer]

<rdar://problem/43786871>