Skip to content

Added expression utility to binaryen-c/.js#1269

Merged
kripken merged 5 commits intoWebAssembly:masterfrom
dcodeIO:js-expression-utility
Nov 11, 2017
Merged

Added expression utility to binaryen-c/.js#1269
kripken merged 5 commits intoWebAssembly:masterfrom
dcodeIO:js-expression-utility

Conversation

@dcodeIO
Copy link
Copy Markdown
Contributor

@dcodeIO dcodeIO commented Nov 7, 2017

Adds the following functions to the C-API:

  • BinaryenExpressionGetId
    Gets the id (kind) of the specified expression.

  • BinaryenExpressionGetType
    Gets the type of the specified expression.

Likewise, these methods are present in the Binaryen namespace in binaryen.js. Its kitchen sink test has been updated accordingly and the recompiled binaries are included. Also exposes all the possible ids.

This is part of multiple PRs as previously tracked in #1261.

Comment thread test/binaryen.js/kitchen-sink.js Outdated
];

// Test expression utility
console.log("ExpressionGetId=" + module.getExpressionId(valueList[3]));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

another option is to put these two methods on Binaryen, as they don't need a module to work. So Binaryen.getExpressionId(..). Thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, though about this as well. Could do either, or both. Ultimately, when thinking about it, one thing that came to my mind was to return instances of a new class Expression, holding a BinaryenExpressionRef, with these expression methods, including print. That'd be quite similar to Module and Relooper, somewhat connected to #1273 and not yet doable without other functions working with references (i.e. could do that for everything, like Globals, Functions, FunctionTypes ...).

Copy link
Copy Markdown
Contributor Author

@dcodeIO dcodeIO Nov 8, 2017

Choose a reason for hiding this comment

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

I don't have a clear preference there because I have switched to using the C-API directly from both JS and WASM, hoping that it becomes interchangeable eventually (see). Not sure if that's an option for upstream as well in the long them, i.e. by making the wrapper just an npm package instead of compiling it in (generating just the raw C-API compiled to both JS and WASM that'd become a lower-level dependency for wrappers, in whatever JS-dialect, to utilize).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Returning a JS class for every expression would be more JS-ey, yeah, but I think it's better not to for perf reasons: we have just one (or a few) Module and Relooper instances, but potentially a huge number of expressions, so my intention was to avoid JS allocation for them.

Not sure I understand your second comment, and first link there is a 404?

Copy link
Copy Markdown
Contributor Author

@dcodeIO dcodeIO Nov 8, 2017

Choose a reason for hiding this comment

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

Yeah, from a performance perspective it might be better to stick to expression references. Considering performance, it might also be better to use real prototypes instead of assigning instance methods through this['emitText'] = etc. as well (haven't benchmarked, probably irrelevant as long as just one or two modules are created).

My idea above was (sorry, updated the links to point to the public repo) to provide just the bare minimum (the raw C-API compiled to JS/WASM) by WebAssembly/binaryen, leaving the respective wrappers, like one using just references or one using classes for everything, up to external modules because there might be more than one wrapper in JS-space anyway, looking at all the languages that transpile to it. For instance, I am using just the static Module exports in the links above, but not the wrapper functions/classes. Just a random thought, of course.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Prototypes might be more standard JS, yeah. Could also help perf, not sure.

Why do you prefer the raw C API exposed to JS instead of the more JS-ey one? If that's more convenient it would be less work, too, maybe I was wrong that we need more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Might be that my requirements are a bit different. Basically, I am speculating on the feasibility of compiling Binaryen to WASM one day, and then being able to simply switch between the two (either calling from JS or linking into a WASM module) with the only common denominator being the raw C-API. In the AssemblyScript case this makes even more sense because the higher level wrapper (2nd link) can be shared between TS and AS as well.

Regarding just providing a minimal API: As I know the crazy people doing npm, there'd probably be multiple wrappers around in a week or two, each focusing on other things (i.e. one being entirely class-based, one being just a few helpers to the C-API and another being written in CoffeScript or something). Not super important, though, because binaryen.js, as it stands, still exposes the underlying raw C-API. Just thinking aloud :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Got it, thanks for the info.

@dcodeIO
Copy link
Copy Markdown
Contributor Author

dcodeIO commented Nov 8, 2017

Hmm... while this now builds, c7e4ee2 should have errored on the binaryen.js/kitchen-sink.js test, which apparently isn't run anymore?

@kripken
Copy link
Copy Markdown
Member

kripken commented Nov 8, 2017

Yeah, the binaryen.js tests weren't run on the bots. I've been working towards that. I just opened #1275 which enables them.

@kripken
Copy link
Copy Markdown
Member

kripken commented Nov 10, 2017

This looks good to land, except for the conflict.

@kripken kripken merged commit 5e4ea07 into WebAssembly:master Nov 11, 2017
@kripken
Copy link
Copy Markdown
Member

kripken commented Nov 11, 2017

Btw, @dcodeIO, if you haven't seen https://github.com/WebAssembly/host-bindings already then you might be interested in that (maybe relevant to AssemblyScript etc.).

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.

2 participants