Enable stream decoding!
Created attachment 348102 [details] Patch
Did you mean to mark this for review?
(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.
Created attachment 348136 [details] Patch
(In reply to Yusuke Suzuki from comment #1) > Created attachment 348102 [details] > Patch Ready for reviews!
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.
Created attachment 348139 [details] Patch
Created attachment 348140 [details] Patch
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.
Created attachment 348141 [details] Patch
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.
Created attachment 348142 [details] Patch
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.
Created attachment 348144 [details] Patch
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 on attachment 348144 [details] Patch OK, ready.
Created attachment 348146 [details] Patch
Created attachment 348147 [details] Patch
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.
Created attachment 348167 [details] Patch
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 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 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.
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.
Committed r235420: <https://trac.webkit.org/changeset/235420>
<rdar://problem/43786871>