Bug 204474 – [WebAssembly] Validate and generate bytecode in one pass

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 384095 [details]
Patch

[Tadeu Zagallo]

Created attachment 384229 [details]
Patch

[Tadeu Zagallo]

Created attachment 384236 [details]
Patch

Fix CMake build

[Saam Barati]

Comment on attachment 384236 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:13
> +        This is a 1.5x speedup when compiling the ZenGarden demo.

what do we do for the .validate API? (You should say here.)

> Source/JavaScriptCore/ChangeLog:14
> +

you should talk about the new policy that WebAssembly.module compiles bytecode

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:237
> +    static_assert(std::is_same_v<ResultList, FunctionParser<AirIRGenerator>::ResultList>);

why not just define one to be the other?

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:89
> +            }

maybe ASSERT here that m_state == Validated

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:146
> +#define WASM_VALIDATOR_FAIL_IF(condition, ...) do { \
> +        if (UNLIKELY(condition))                    \
> +            return validationFail(__VA_ARGS__);               \
> +    } while (0)

style nit: line up the "\" or have them be one space after

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:288
> +    WASM_VALIDATOR_FAIL_IF(pointer.type() != I32, m_currentOpcode, " pointer type mismatch");

maybe instead of "type mismatch" say " pointer type must be an i32"?
(This is probably not new to your patch though, so feel free to ignore)

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:311
> +    WASM_VALIDATOR_FAIL_IF(pointer.type() != I32, m_currentOpcode, " pointer type mismatch");

ditto

> Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp:58
> +{

ASSERT m_callees?

[Tadeu Zagallo]

Comment on attachment 384236 [details]
Patch

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

Thanks for the review!

>> Source/JavaScriptCore/ChangeLog:13
>> +        This is a 1.5x speedup when compiling the ZenGarden demo.
> 
> what do we do for the .validate API? (You should say here.)

done

>> Source/JavaScriptCore/ChangeLog:14
>> +
> 
> you should talk about the new policy that WebAssembly.module compiles bytecode

done

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:237
>> +    static_assert(std::is_same_v<ResultList, FunctionParser<AirIRGenerator>::ResultList>);
> 
> why not just define one to be the other?

I want it to be defined in FunctionParser, and we can't define it here as FunctionParser<AirIRGenerator>::ResultList because that will fail to instantiate FunctionParser, since that requires ControlType to be defined and ControlType depends on ResultList.

>> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:89
>> +            }
> 
> maybe ASSERT here that m_state == Validated

added

>> Source/JavaScriptCore/wasm/WasmFunctionParser.h:146
>> +    } while (0)
> 
> style nit: line up the "\" or have them be one space after

fixed

>> Source/JavaScriptCore/wasm/WasmFunctionParser.h:288
>> +    WASM_VALIDATOR_FAIL_IF(pointer.type() != I32, m_currentOpcode, " pointer type mismatch");
> 
> maybe instead of "type mismatch" say " pointer type must be an i32"?
> (This is probably not new to your patch though, so feel free to ignore)

I will fix this in a separate patch since there are a lot of existing code that needs fixing.

>> Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp:58
>> +{
> 
> ASSERT m_callees?

added

[Tadeu Zagallo]

Created attachment 384859 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 384859 [details]
Patch for landing

Rejecting attachment 384859 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 384859, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
eeded at 1 with fuzz 3.
patching file Source/JavaScriptCore/CMakeLists.txt
Hunk #1 succeeded at 366 (offset -2 lines).
patching file Source/JavaScriptCore/DerivedSources-input.xcfilelist
patching file Source/JavaScriptCore/DerivedSources-output.xcfilelist
Hunk #1 succeeded at 60 with fuzz 1 (offset -1 lines).
patching file Source/JavaScriptCore/DerivedSources.make
Hunk #2 succeeded at 353 (offset 2 lines).
patching file Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp
Hunk #18 succeeded at 1799 (offset 77 lines).
Hunk #19 succeeded at 1830 (offset 77 lines).
Hunk #20 succeeded at 1843 (offset 77 lines).
Hunk #21 succeeded at 1861 (offset 77 lines).
Hunk #22 succeeded at 1877 (offset 77 lines).
Hunk #23 succeeded at 1898 (offset 77 lines).
Hunk #24 succeeded at 1936 (offset 77 lines).
Hunk #25 succeeded at 2051 (offset 77 lines).
Hunk #26 succeeded at 2068 (offset 77 lines).
Hunk #27 succeeded at 2093 (offset 77 lines).
Hunk #28 succeeded at 2183 (offset 77 lines).
Hunk #29 succeeded at 2339 (offset 77 lines).
Hunk #30 succeeded at 2357 (offset 77 lines).
Hunk #31 succeeded at 2382 (offset 77 lines).
Hunk #32 succeeded at 2405 (offset 77 lines).
Hunk #33 succeeded at 2994 (offset 77 lines).
Hunk #34 succeeded at 3069 (offset 77 lines).
patching file Source/JavaScriptCore/wasm/WasmAirIRGenerator.h
patching file Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp
Hunk #14 succeeded at 1291 (offset 68 lines).
Hunk #15 succeeded at 1354 (offset 68 lines).
Hunk #16 succeeded at 1390 (offset 68 lines).
Hunk #17 succeeded at 1409 (offset 68 lines).
Hunk #18 succeeded at 1437 (offset 68 lines).
Hunk #19 succeeded at 1456 (offset 68 lines).
Hunk #20 succeeded at 1476 (offset 68 lines).
Hunk #21 succeeded at 1547 (offset 68 lines).
Hunk #22 succeeded at 1568 (offset 68 lines).
Hunk #23 succeeded at 1919 (offset 68 lines).
Hunk #24 succeeded at 1954 (offset 68 lines).
Hunk #25 succeeded at 1978 (offset 68 lines).
patching file Source/JavaScriptCore/wasm/WasmB3IRGenerator.h
patching file Source/JavaScriptCore/wasm/WasmBBQPlan.cpp
patching file Source/JavaScriptCore/wasm/WasmBBQPlan.h
patching file Source/JavaScriptCore/wasm/WasmCodeBlock.cpp
patching file Source/JavaScriptCore/wasm/WasmCodeBlock.h
patching file Source/JavaScriptCore/wasm/WasmEntryPlan.cpp
patching file Source/JavaScriptCore/wasm/WasmEntryPlan.h
patching file Source/JavaScriptCore/wasm/WasmFunctionParser.h
patching file Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp
Hunk #1 succeeded at 49 (offset 1 line).
Hunk #2 succeeded at 72 (offset 1 line).
Hunk #3 succeeded at 92 (offset 1 line).
Hunk #4 succeeded at 105 (offset 1 line).
Hunk #5 succeeded at 137 (offset 1 line).
Hunk #6 succeeded at 154 (offset 1 line).
Hunk #7 succeeded at 182 (offset 1 line).
Hunk #8 succeeded at 234 (offset 1 line).
Hunk #9 succeeded at 250 (offset 1 line).
Hunk #10 succeeded at 293 (offset 1 line).
Hunk #11 succeeded at 363 (offset 1 line).
Hunk #12 succeeded at 408 (offset 1 line).
Hunk #13 succeeded at 433 (offset 1 line).
Hunk #14 succeeded at 525 (offset 1 line).
Hunk #15 succeeded at 590 (offset 1 line).
Hunk #16 succeeded at 696 (offset 1 line).
Hunk #17 succeeded at 756 (offset 1 line).
Hunk #18 succeeded at 806 (offset 20 lines).
Hunk #19 succeeded at 825 (offset 20 lines).
Hunk #20 succeeded at 843 (offset 20 lines).
Hunk #21 succeeded at 853 (offset 20 lines).
Hunk #22 succeeded at 973 (offset 20 lines).
Hunk #23 succeeded at 1005 (offset 20 lines).
Hunk #24 succeeded at 1019 (offset 20 lines).
patching file Source/JavaScriptCore/wasm/WasmLLIntGenerator.h
patching file Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp
patching file Source/JavaScriptCore/wasm/WasmLLIntPlan.h
patching file Source/JavaScriptCore/wasm/WasmModule.cpp
patching file Source/JavaScriptCore/wasm/WasmModule.h
patching file Source/JavaScriptCore/wasm/WasmOMGPlan.cpp
patching file Source/JavaScriptCore/wasm/WasmPlan.cpp
patching file Source/JavaScriptCore/wasm/WasmPlan.h
patching file Source/JavaScriptCore/wasm/WasmSlowPaths.cpp
Hunk #1 succeeded at 411 (offset 7 lines).
patching file Source/JavaScriptCore/wasm/WasmThunks.cpp
patching file Source/JavaScriptCore/wasm/WasmThunks.h
patching file Source/JavaScriptCore/wasm/WasmValidate.cpp
Hunk #4 FAILED at 170.
1 out of 8 hunks FAILED -- saving rejects to file Source/JavaScriptCore/wasm/WasmValidate.cpp.rej
patching file Source/JavaScriptCore/wasm/WasmWorklist.cpp
patching file Source/JavaScriptCore/wasm/generateWasmOpsHeader.py
patching file Source/JavaScriptCore/wasm/generateWasmValidateInlinesHeader.py
rm 'Source/JavaScriptCore/wasm/generateWasmValidateInlinesHeader.py'
patching file Source/JavaScriptCore/wasm/js/JSWebAssembly.cpp
Hunk #1 succeeded at 229 (offset 1 line).
Hunk #2 succeeded at 329 (offset 1 line).
patching file Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp
Hunk #1 succeeded at 81 with fuzz 1.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/13286559

[Tadeu Zagallo]

Created attachment 384874 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 384874 [details]
Patch for landing

Clearing flags on attachment: 384874

Committed r253140: <https://trac.webkit.org/changeset/253140>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57652345>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/253140/webkit

caused 11 tests to crash on Mac debug wk1.

tracking in https://bugs.webkit.org/show_bug.cgi?id=204893