[webkit-reviews] review granted: [Bug 188943] [WebAssembly] Parse wasm modules in a streaming fashion : [Attachment 348167] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<yusukesuzuki at slowstart.org>'s request for review:
Bug 188943: [WebAssembly] Parse wasm modules in a streaming fashion
https://bugs.webkit.org/show_bug.cgi?id=188943

Attachment 348167: Patch

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




--- Comment #22 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 348167
  --> https://bugs.webkit.org/attachment.cgi?id=348167
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/.