Bug 222412 – Add type method for WASM Global, Memory and Table JS API classes

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

Supporting the type method on WebAssembly.Global, WebAssembly.Memory and WebAssembly.Table in the wasm JS-API would allow us to inspect the type of those objects as well as create new objects with the same type by passing the type return by the new type method to the constructor. This is outlined in https://github.com/WebAssembly/js-types/blob/master/proposals/js-types/Overview.md#extensions-to-api-functions

Created attachment 421514 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Yusuke Suzuki]

Comment on attachment 421514 [details]
Patch

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

Commented.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyGlobal.cpp:115
> +    default:
> +        RELEASE_ASSERT_NOT_REACHED();
> +    }

We need to handle all types that are supported in WebAssembly.Global (e.g. funcref).

> Source/JavaScriptCore/wasm/js/WebAssemblyGlobalPrototype.cpp:90
> +    return JSValue::encode(global->type(globalObject));

Use
RELEASE_AND_RETURN(throwScope, JSValue::encode(global->type(globalObject)));

> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryPrototype.cpp:112
> +    JSObject* memoryType = memory->type(globalObject);
> +    RETURN_IF_EXCEPTION(throwScope, { });
> +
> +    RELEASE_AND_RETURN(throwScope, JSValue::encode(memoryType));

Do,

RELEASE_AND_RETURN(throwScope, JSValue::encode(memory->type(globalObject)));

> Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:178
> +    JSObject* tableType = table->type(globalObject);
> +    return JSValue::encode(tableType);

Do

RELEASE_AND_RETURN(throwScope, JSValue::encode(table->type(globalObject)));

[Radar WebKit Bug Importer]

<rdar://problem/75030783>

Created attachment 422927 [details]
Patch

Created attachment 423326 [details]
Patch

Created attachment 423340 [details]
Patch

This is ready for review. I saw that some windows tests are failing but taking a look at the crashed test I don't think it's related to my patch.

[Yusuke Suzuki]

(In reply to jtallon from comment #8)
> This is ready for review. I saw that some windows tests are failing but
> taking a look at the crashed test I don't think it's related to my patch.

Could you set r? for the patch requesting the review?

[Caio Lima]

Comment on attachment 423340 [details]
Patch

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

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:139
> +    JSObject* result = constructEmptyObject(globalObject, globalObject->objectPrototype(), 2);

We are setting `inlineCapacity` to 2 here, but there are cases where we will potentially have 3 properties. This will make such object allocate a butterfly when we have the third property, increasing the amount of memory we use for this object, and slowing down the access of such property (because it will be stored inside the butterfly). Since `maximum` property is conditional, I think we could move the creation of `result` after we know if  it will be included or not.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:133
> +    JSObject* result = constructEmptyObject(globalObject, globalObject->objectPrototype(), 3);

Ditto here. But in this case we are setting 3 while we don't know if `maximum` is going to be present. In such situation, we are allocating some space that might be useless.

Created attachment 423589 [details]
Patch

[Caio Lima]

Comment on attachment 423589 [details]
Patch

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

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:143
> +    auto maximum = m_table->maximum();

I have a legit question. Is there any reason why `JSWebAssemblyMemory::maximum` can throw, but `JSWebAssemblyTable::maximum` doesn't?

[Yusuke Suzuki]

Comment on attachment 423589 [details]
Patch

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

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:140
> +    RETURN_IF_EXCEPTION(throwScope, { });

Yes, this will not throw an error. Let's remove it.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:143
> +    RETURN_IF_EXCEPTION(throwScope, { });

Ditto.

Created attachment 424329 [details]
Patch

Created attachment 424533 [details]
Patch

Created attachment 424625 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 424625 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/wasm/js/JSWebAssemblyGlobal.cpp:96
> +    result->putDirect(vm, Identifier::fromString(vm, "mutable"), jsBoolean(isMutable));

Use

result->putDirect(vm, Identifier::fromString(vm, "mutable"), jsBoolean(m_global->mutability() == Wasm::GlobalInformation::Mutable));

> Source/JavaScriptCore/wasm/js/JSWebAssemblyGlobal.cpp:98
> +    Wasm::Type value = m_global->type();

valueType would be better.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyGlobal.cpp:99
> +    JSString* valueStr = nullptr;

Use `valueString`.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:154
> +    Wasm::MemorySharingMode shared = m_memory->sharingMode();
> +    if (shared == Wasm::MemorySharingMode::Default)
> +        result->putDirect(vm, Identifier::fromString(vm, "shared"), jsBoolean(false));
> +    else
> +        result->putDirect(vm, Identifier::fromString(vm, "shared"), jsBoolean(true));

Make it,

result->putDirect(vm, Identifier::fromString(vm, "shared"), jsBoolean(m_memory->sharingMode() == Wasm::MemorySharingMode::Shared));

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:134
> +    JSString* elementStr = nullptr;

Use `elementString` instead. We like non-abbreviated names.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:138
> +        elementStr = jsNontrivialString(vm, "externref");

Let's make it switch to list things in a readable way.

JSString* elementString = nullptr;
switch (element) {
case Wasm::TableElementType::Funcref:
    elementString = jsNontrivialString(vm, "funcref");
    break;
case Wasm::TableElementType::Externref:
    elementString = jsNontrivialString(vm, "externref");
    break;
default:
    RELEASE_ASSERT_NOT_REACHED();
    break;
}

Created attachment 424758 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 424758 [details]
Patch

r=me, nice!

[EWS]

Committed r275438: <https://commits.webkit.org/r275438>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424758 [details].