1782770 - Transfer all type definitions to Metadata

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, task, P2)

Description

[Ryan Hunt [:rhunt]]

Metadata stores a vector of TypeDef that comes from the type section of the module. We currently only transfer a type if it's a struct type, array type, or else a function type that cannot fit in an immediate for call_indirect signature checks.

Because we filter out some types, this means we need to renumber the type index space to match. This is a bit hacky, and is difficult to do consistently.

The original reason for only bringing function types that cannot fit in an immediate was to reduce the size of metadata as most function types can fit in an immediate. However, I discovered that we make a copies of function types for FuncExport and FuncImport metadata. This can create multiple copies of the original function type, e.g. when multiple exported functions reference the same type definition.

If we instead transfer all types and store the index of the function type in FuncExport/FuncImport we can reduce total metadata size, reduce allocations, and remove type renumbering.

Comment 1

[Ryan Hunt [:rhunt]]

Attachment: Bug 1782770 - wasm: Transfer all type definitions to Metadata. r?yury

Metadata stores a vector of TypeDef that comes from the type section of the module.
We currently only transfer a type if it's a struct type, array type, or else a
function type that cannot fit in an immediate for call_indirect signature checks.

Because we filter out some types, this means we need to renumber the type index
space to match. This is a bit hacky, and is difficult to do consistently.

The original reason for only bringing function types that cannot fit in an
immediate was to reduce the size of metadata as most function types can fit in an
immediate. However, I discovered that we make a copies of function types for
FuncExport and FuncImport metadata. This can create multiple copies of the original
function type, e.g. when multiple exported functions reference the same type
definition.

If we instead transfer all types and store the index of the function type in
FuncExport/FuncImport we can reduce total metadata size, reduce allocations,
and remove type renumbering.

This commit does this by:

1. Transferring all type definitions to Metadata (WasmGenerator.cpp)
2. Removing the typeRenumbering vector
3. Replacing FuncType with typeIndex on FuncImport/FuncExport
4. Updating all users of FuncImport/FuncExport to get the FuncType using
   the typeIndex and Metadata. This the bulk of the changes.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: Bug 1782770 - wasm: Make ValType a uint32_t now that rtt's are gone. r?yury

ValType needed to be pointer size to support (rtt) types. Now that
they have been removed, we can revert to 32-bit on all platforms.

Depends on D153498

Comment 3

[Ryan Hunt [:rhunt]]

Attachment: Bug 1782770 - wasm: Don't make extra copy of FuncType for debugging. r?yury

Now that we transfer all type definitions to Metadata, we can
remove the special debugging case where we would transfer all
function types. Instead, we can just transfer the funcTypeIndex
and find the function type in Metadata.

Depends on D153499

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a881b6ab4795
wasm: Transfer all type definitions to Metadata. r=yury
https://hg.mozilla.org/integration/autoland/rev/c4eca5ce1a52
wasm: Make ValType a uint32_t now that rtt's are gone. r=yury
https://hg.mozilla.org/integration/autoland/rev/dfb8854e3ac0
wasm: Don't make extra copy of FuncType for debugging. r=yury

Comment 5

[Iulian Moraru]

Backed out for causing wasm related spidermonkey build bustages.

Push with failures

Failure log

Backout link

[task 2022-08-11T22:04:10.312Z] gmake[4]: Entering directory '/builds/worker/workspace/obj-spider/js/src/wasm'
[task 2022-08-11T22:04:10.313Z] /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -std=gnu++17 -o WasmDebugFrame.o -c  -I/builds/worker/workspace/obj-spider/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -ftrivial-auto-var-init=pattern -DDEBUG=1 -DWASM_SUPPORTS_HUGE_MEMORY -DJS_CACHEIR_SPEW -DJS_STRUCTURED_SPEW -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -I/builds/worker/checkouts/gecko/js/src/wasm -I/builds/worker/workspace/obj-spider/js/src/wasm -I/builds/worker/workspace/obj-spider/js/src -I/builds/worker/checkouts/gecko/js/src -I/builds/worker/workspace/obj-spider/dist/include -I/builds/worker/workspace/obj-spider/dist/include/nspr -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-spider/js/src/js-confdefs.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wdeprecated-this-capture -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wenum-compare-conditional -Wno-ambiguous-reversed-operator -Wno-error=deprecated -Wno-error=deprecated-anon-enum-enum-conversion -Wno-error=deprecated-enum-enum-conversion -Wno-error=deprecated-enum-float-conversion -Wno-error=deprecated-pragma -Wno-error=deprecated-this-capture -Wno-error=deprecated-volatile -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=free-nonheap-object -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-error=deprecated-copy -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-psabi -Wthread-safety -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-spider/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -Werror=format -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/WasmDebugFrame.o.pp   /builds/worker/checkouts/gecko/js/src/wasm/WasmDebugFrame.cpp
[task 2022-08-11T22:04:10.313Z] In file included from /builds/worker/checkouts/gecko/js/src/wasm/WasmDebugFrame.cpp:21:
[task 2022-08-11T22:04:10.313Z] In file included from /builds/worker/checkouts/gecko/js/src/vm/EnvironmentObject.h:17:
[task 2022-08-11T22:04:10.313Z] In file included from /builds/worker/checkouts/gecko/js/src/vm/GlobalObject.h:33:
[task 2022-08-11T22:04:10.313Z] In file included from /builds/worker/checkouts/gecko/js/src/vm/JSContext.h:36:
[task 2022-08-11T22:04:10.313Z] In file included from /builds/worker/checkouts/gecko/js/src/vm/Runtime.h:72:
[task 2022-08-11T22:04:10.313Z] In file included from /builds/worker/checkouts/gecko/js/src/vm/Scope.h:38:
[task 2022-08-11T22:04:10.313Z] In file included from /builds/worker/checkouts/gecko/js/src/wasm/WasmJS.h:41:
[task 2022-08-11T22:04:10.313Z] In file included from /builds/worker/checkouts/gecko/js/src/wasm/WasmCodegenTypes.h:31:
[task 2022-08-11T22:04:10.314Z] In file included from /builds/worker/checkouts/gecko/js/src/wasm/WasmInstanceData.h:27:
[task 2022-08-11T22:04:10.314Z] /builds/worker/checkouts/gecko/js/src/wasm/WasmInstance.h:317:26: error: inline function 'js::wasm::Instance::metadata' is not defined [-Werror,-Wundefined-inline]
[task 2022-08-11T22:04:10.314Z]   inline const Metadata& metadata() const;
[task 2022-08-11T22:04:10.314Z]                          ^
[task 2022-08-11T22:04:10.314Z] /builds/worker/checkouts/gecko/js/src/wasm/WasmDebugFrame.cpp:138:19: note: used here
[task 2022-08-11T22:04:10.314Z]       instance()->metadata().debugFuncType(funcIndex()).results());
[task 2022-08-11T22:04:10.314Z]                   ^
[task 2022-08-11T22:04:10.314Z] 1 error generated.
[task 2022-08-11T22:04:10.314Z] gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:669: WasmDebugFrame.o] Error 1
[task 2022-08-11T22:04:10.314Z] gmake[4]: Leaving directory '/builds/worker/workspace/obj-spider/js/src/wasm'
[task 2022-08-11T22:04:10.314Z] gmake[4]: *** Waiting for unfinished jobs....
[task 2022-08-11T22:04:10.318Z] gmake[4]: Entering directory '/builds/worker/workspace/obj-spider/js/src/gc'

Comment 6

[Ryan Hunt [:rhunt]]

Non-unified builds..

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c867a71812f8
wasm: Transfer all type definitions to Metadata. r=yury
https://hg.mozilla.org/integration/autoland/rev/4f2d7c625c8d
wasm: Make ValType a uint32_t now that rtt's are gone. r=yury
https://hg.mozilla.org/integration/autoland/rev/115c87e6a8bd
wasm: Don't make extra copy of FuncType for debugging. r=yury

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/c867a71812f8
https://hg.mozilla.org/mozilla-central/rev/4f2d7c625c8d
https://hg.mozilla.org/mozilla-central/rev/115c87e6a8bd