Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add detector for type reflection #67

Merged
merged 2 commits into from
May 16, 2023

Conversation

hoodmane
Copy link
Contributor

No description provided.

Copy link
Collaborator

@surma surma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make the detection a bit more robust.

Also, can you please make sure that there are no whitespace changes introduced by your editor?

*/

export default async () => {
return "Function" in WebAssembly;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think this is enough. IINM, WebAssembly.Function is also expose by the JSPI proposal.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this could work?

return "Function" in WebAssembly && typeof WebAssembly["Function"]["type"] === "function" &&
    typeof WebAssembly.Memory.prototype["type"] === "function" &&
    typeof WebAssembly.Table.prototype["type"] === "function" &&
    typeof WebAssembly.Global.prototype["type"] === "function";

Doing some tests:

  • Chrome: Returns true with experimental wasm features enabled
  • Safari: Memory, Table and Global have the type() method, but it does not have WebAssembly.Function and functions exported from a WebAssembly.Instance do not have the type() method
  • Firefox: All conditions are false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try it with d8 with various extra feature flags.

IINM, WebAssembly.Function is also expose by the JSPI proposal.

As I understand it, JSPI implies type reflection. E.g., passing --experimental-wasm-stack-switching --experimental-type-reflection is indistinguishable from just --experimental-wasm-stack-switching. To test this I made a little Python script:

I made feature_detect.js:

console.log(
  "Function" in WebAssembly,
  "Function" in WebAssembly &&
    typeof WebAssembly["Function"]["type"] === "function" &&
    typeof WebAssembly.Memory.prototype["type"] === "function" &&
    typeof WebAssembly.Table.prototype["type"] === "function" &&
    typeof WebAssembly.Global.prototype["type"] === "function"
);

and then tested with V8 version 11.5.69

$ v8 --experimental-wasm-type-reflection feature_detect.js
true true
$ v8 --experimental-wasm-stack-switching feature_detect.js
true true
$ v8 feature_detect.js
false false

For good measure I listed out all experimental flags with:

v8 --help | grep -o -E -- '--experimental[a-z0-9-]*' | sort -u

and checked that the script returns true true if either of these two flags is present and false false with any other collection of experimental flags. (Well I picked the other flags at random so I only tested a random sample.)

Test script
import subprocess
from random import randrange
from itertools import product
import sys

result = subprocess.run(
    "v8 --help | grep -o -E -- '--experimental-[a-z0-9-]*' | sort -u",
    shell=True,
    capture_output=True,
    encoding="utf8",
    check=True
)

experimental_flags = set([x for x in result.stdout.splitlines() if x])
experimental_flags.discard("--experimental-wasm-stack-switching")
experimental_flags.discard("--experimental-wasm-type-reflection")


def random_flags():
    return set(filter(lambda x: randrange(2), experimental_flags))


for [stack_switch, type_refl] in product([False, True], repeat=2):
    if stack_switch or type_refl:
        expected = "true true"
    else:
        expected = "false false"
    flags = []
    if stack_switch:
        flags.append("--experimental-wasm-stack-switching")
    if type_refl:
        flags.append("--experimental-wasm-type-reflection")
    for i in range(1000):
        rand_flags = random_flags()
        r = subprocess.run(
            ["/home/hood/.jsvu/v8", *flags, *rand_flags, "a.js"],
            capture_output=True,
            encoding="utf8",
        )
        if r.stdout.strip() != expected:
            from pprint import pprint
            pprint(flags + list(rand_flags))
            sys.exit(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the moral of the story is that both approaches should do the same thing, at least on v8. Node is a similar story. I guess I should check also with firefox...

Anyways if you folks prefer, I am fine with switching to the more detailed check. I think we should add a comment though.

See also Emscripten's feature detection:

    // If the type reflection proposal is available, use the new
    // "WebAssembly.Function" constructor.
    // Otherwise, construct a minimal wasm module importing the JS function and
    // re-exporting it.
    if (typeof WebAssembly.Function == "function") {

https://github.com/emscripten-core/emscripten/blob/main/src/library_addfunction.js#L98-L105

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbc100 do you have an opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I guess I may be overly focusing on v8 checks here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the check as written seems like it should be fine.

JSPI depends on the type reflection proposal, but I don' think that needs to effect this test. JSPI itself doesn't provide this symbol.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahah: in firefox nightly v115.0a1 I get:

true false

but if I fix:

    typeof WebAssembly["Function"]["type"] === "function" &&

to

    typeof WebAssembly.Function.prototype["type"] === "function" &&

then I get true true again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Safari Version 16.3 (18614.4.6.1.6),

console.log(
  "Function" in WebAssembly,
  "Function" in WebAssembly &&
    typeof WebAssembly.Function.protype.type === "function",
    typeof WebAssembly.Memory.prototype["type"] === "function",
    typeof WebAssembly.Table.prototype["type"] === "function",
    typeof WebAssembly.Global.prototype["type"] === "function"
);

gives false false true true true as @juancastillo0 said.

In Safari Technology Preview Release 169 (Safari 16.4, WebKit 18616.1.12.2) I get the same thing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks everyone! Sounds like we can leave this as is!

@hoodmane
Copy link
Contributor Author

make sure that there are no whitespace changes introduced by your editor?

@surma I am confused about the whitespace -- as far as I can tell README.md is a generated file. I committed exactly what npm run build generated. Is is possible that I am getting the wrong version of some dependency? I used npm ci. As far as I can tell README.md.ejs has spaces in it but README.md has tabs...

@surma
Copy link
Collaborator

surma commented May 16, 2023

I assumed that maybe your editor ran a different formatter. If that is not happening, ignore the whitespace problem 🤷

@surma surma merged commit b34b628 into GoogleChromeLabs:main May 16, 2023
@hoodmane hoodmane deleted the type-reflection branch May 17, 2023 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants