Update wasm2asm to generate "almost asm" if grow_memory is used #1340

krisselden · 2017-12-19T22:58:36Z

Emits "almost asm" if grow_memory, current_memory or export memory is used.

kripken

Thanks!

Do we need a flag here? Perhaps if memory growth is used, we can automatically do this and emit almost asm?

kripken · 2017-12-20T17:46:26Z

test/wasm2asm.memory.js

+ }
+
+ return {
+  memory: Object.create(Object.prototype, {


why is this Object stuff needed?

Because buffer needs to be a getter over the buffer var so we are exporting a living binding and Object.create supports an array of descriptors.

krisselden · 2017-12-20T21:59:18Z

I will refactor to just automatically do it based on the presence of grow_memory and I'll fix the python linting rules.

I used Object.create() because it is how you create with descriptors and need the buffer to be a getter so it reflects the replacement when memory grows. I could use the new es2015 shorthand instead get buffer() { return buffer; } but not sure if the cashew ast supported that. I could also assign memory as a var, and just ensure memory.buffer = buffer is part of replacement, but this way I don't create the memory object unless it is exported.

wycats · 2017-12-28T02:03:39Z

This would be great to have for Glimmer, where we're targeting wasm but still need asm.js output for older browsers (we still support IE11, for now).

kripken · 2017-12-28T02:42:14Z

src/wasm2asm.h

@@ -205,11 +205,14 @@ class Wasm2AsmBuilder {
  // All our function tables have the same size TODO: optimize?
  size_t tableSize;

+  bool almostASM;


initializing this to false could be done here, in the definition.

kripken · 2017-12-28T02:43:50Z

src/wasm2asm.h

+      );
+    }
+    if (export_->kind == ExternalKind::Memory) {
+      almostASM = true;


I don't think this should depend on the presence of a memory export - maybe I'm missing something?

I think it should happen of growth is used, as I see later in the patch, and also the memory has an initial and a max value; if the max is larger than the initial, then growth is possible (from outside - maybe we don't care about that?).

I don't think asm.js exports can be anything other than functions.

http://asmjs.org/spec/latest/#validateexport-s

Is enforcing the maximum growth important for a first pass? The primary motivation is getting backward compatibility when targeting wasm first.

I see what you mean now, yeah, asm.js doesn't allow exporting the memory. However, I think we should just not export the memory - asm.js always imports (and never exports) the memory, no matter what wasm does, so that is always going to be different.

About enforcing the maximum growth, I agree, we can leave that for later.

The export shims the WebAssembly.Memory interface and is also the only way to get the reassigned buffer post memory growth, the imported buffer is only the initial buffer.

I see, thanks, then I agree we need to export it and disable "use asm".

However, can we not do this if memory growth is disabled? I think it may be common to have a non-memory growth wasm module that exports a memory, and it would be better to not export it so that we can keep "use asm" for speed. But, a downside of that is we do something different depending on memory growth. Perhaps we can issue a warning, and/or have an option to ignore a memory export?

Exporting memory allows for memory growth externally, via the grow function even if the module itself doesn't have grow_memory instructions. I will log a message when "almost asm" is triggered with the reason why.

I'm excited for this!

Ok, this looks great! All ready to merge I think, except for the logging just mentioned, which I don't think I see yet?

If grow_memory, current_memory or export memory is used then generate "almost asm" with memory growth support.

kripken · 2018-01-05T00:10:15Z

Thanks, looks good. Before we merge this, please join the wasm w3c community group https://www.w3.org/community/webassembly/participants

wycats · 2018-01-05T00:49:07Z

While I'm sure @krisselden is happy to fill out another form, I'm curious @kripken: is there a reason that contributions to binaryen require joining a community group?

kripken · 2018-01-05T01:10:07Z

I don't understand it myself, but it's what the WebAssembly group wanted, and this repo is under that org. It may have something to do with the w3c, @jfbastien knows all the details.

dschuff · 2018-01-05T01:13:56Z

The CG provides the legal protections for all technical contributions to WebAssembly and its projects. Most notably patent protection: see https://www.w3.org/community/about/agreements/summary/ for an overview and https://www.w3.org/community/about/agreements/cla/ for details.

wycats · 2018-01-05T01:27:56Z

Is there some reason that this tool needs to live in this org and be under the W3C IPR policy (as opposed to a standalone repository licensed under Apache2?)

Are any of the active contributors to this repository only active because of the fact that the repository is part of what WebAssembly umbrella and protected (specifically) by W3C IPR?

jfbastien · 2018-01-09T21:16:58Z

@wycats sorry for missing this discussion. The title made me mark-as-read. Please consider forking this question to its own thread instead of tacking it onto this one.

To answer your question: binaryen was started early in WebAssembly's history when much of the design was in flux, and it definitely provided significant input to what will become the final WebAssembly standard. I was one of the 4 CG chairs at the time, representing Google, and advice from representative companies' standards folks + lawyers, and W3C folks, was that WebAssembly contributions which affect the design must be under the W3C umbrella and contributors who provide significant input must be CG members. Binaryen fit that description and was therefore under the WebAssembly organization.

Now that the design has settled more we could reconsider and put binaryen as well as other tools under their own organization. For example, the LLVM work now occurs directly in upstream LLVM. It remains true that significant contributions to the standard must come from CG members, but at this point tooling development doesn't influence the design as much as just get stuff working.

If you want to float this idea I recommend filing an issue here, and adding an agenda item for the next CG video call (today's is a bit late, but I'll soon post an agenda for the next meeting, at which point a PR for this can be created). CG video calls are open to all CG members, I suggest this path because it seems like the choice of un-CG-ifying binaryen should be put to CG members.

krisselden · 2018-01-30T22:16:01Z

Microsoft already has signed a contribution agreement (as a member of the W3C), and we believe we are covered by that signed agreement. So can we submit without signing another agreement?

kripken · 2018-02-01T20:54:52Z

ping @jfbastien for that legal question.

kripken · 2018-02-03T02:53:02Z

Well, to my understanding that should be more than sufficient. I'll merge this in Monday unless someone objects.

Thanks for your work and your patience here @krisselden!

kripken reviewed Dec 20, 2017

View reviewed changes

krisselden force-pushed the almost-asm branch from 50d0593 to ce04f4c Compare December 28, 2017 00:14

krisselden changed the title ~~Add --allow-memory-growth to wasm2asm to generate "almost asm" js~~ Update wasm2asm to generate "almost asm" if grow_memory is used Dec 28, 2017

kripken reviewed Dec 28, 2017

View reviewed changes

Allow wasm2asm to generate "almost asm"

e60dcb9

If grow_memory, current_memory or export memory is used then generate "almost asm" with memory growth support.

krisselden force-pushed the almost-asm branch from ce04f4c to e60dcb9 Compare December 28, 2017 21:33

Log reason for "almost asm" to stderr

9c6cd74

wycats mentioned this pull request Jan 11, 2018

Consider moving binaryen (and other tools) into a different org #1358

Open

kripken merged commit a85be97 into WebAssembly:master Feb 6, 2018

krisselden mentioned this pull request Mar 14, 2018

Patches for wasm2asmjs #1333

Closed

Update wasm2asm to generate "almost asm" if grow_memory is used #1340

Update wasm2asm to generate "almost asm" if grow_memory is used #1340

Uh oh!

Conversation

krisselden commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krisselden commented Dec 20, 2017

Uh oh!

wycats commented Dec 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Jan 5, 2018

Uh oh!

wycats commented Jan 5, 2018

Uh oh!

kripken commented Jan 5, 2018

Uh oh!

dschuff commented Jan 5, 2018

Uh oh!

wycats commented Jan 5, 2018

Uh oh!

jfbastien commented Jan 9, 2018

Uh oh!

krisselden commented Jan 30, 2018

Uh oh!

kripken commented Feb 1, 2018

Uh oh!

kripken commented Feb 3, 2018

Uh oh!

Uh oh!

krisselden commented Dec 19, 2017 •

edited

Loading