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

Consolidate explanation of modules into a new Modules.md and improve explanation #270

Merged
merged 5 commits into from
Aug 3, 2015

Conversation

lukewagner
Copy link
Member

This PR is an attempt to consolidate the discussion of modules and their integration with ES6 modules into one place and also provide a better explanation.

Independently, there's been some agreement to work on a new "level 0" version of the loader spec with the goal of being "just enough to be able to ship ES6 modules and WebAssembly modules". When there is a URL to point to, I'll update the TODO link in Modules.md.

Especially interested for feedback from @ajklein.

(On a side note, with this PR, MVP.md is now pretty empty and ad hoc, it'd be good to tidy it up. I might do that if noone beats me to it.)

* [Polyfill to JavaScript](Polyfill.md);
* [AST semantics](AstSemantics.md);
* [Binary encoding](BinaryEncoding.md);
* Implementation [in the browser](Web.md) and [outside the browser](NonWeb.md).

**Note**: This content is still in flux and open for discussion.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep this note for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to, I was just thinking that this was originally added when V1.md had a bunch of text and so now it seems a bit ad hoc and asymmetric since most files don't have it. The important thing is the Readme.md has it bold. Still want to keep?

@lukewagner
Copy link
Member Author

@jfbastien Which line are you referring to?

@jfbastien
Copy link
Member

Hmm, it looks like commenting on an incremental diff doesn't link well :-s
Chasing links:
17d32b8#commitcomment-12235834

@lukewagner
Copy link
Member Author

Updated to address feedback.

@jfbastien
Copy link
Member

This mostly lgtm, but I'd like @ncbray's concerns to be captured somewhere: I think there are still open issues as @ncbray pointed out. Overall this clarifies things, so I like it, but I don't want to paper over issues!

@lukewagner
Copy link
Member Author

I thought that I had; are there any outstanding issues not addressed in the current version?

@lukewagner
Copy link
Member Author

@ncbray Rebased to trunk

@ncbray
Copy link

ncbray commented Jul 28, 2015

This is the only way I found to view a complete diff:
https://github.com/WebAssembly/design/compare/better-explain-modules
but it isn’t linked to the PR. Is there a better way? Out-of-line comments, because of that.

In general I think the consolidation, editing, and clarification is good. The new ideas should probably be discussed with a wider audience?

At first glance, WASM importing WASM being type checked is potentially contentious/complicated. It is a weird corner case, normally imports are “whatever the embedder wants” and this burns a hole that seems tricky to define. There are also crazier things I’ve been thinking about, such as allowing a signed/unsigned distinction in imports so values could be fixed up before passing them to JS. How does that interact? It does seem reasonable that the implementation could type check imports to some degree (WASM or otherwise) but ironing out the details... hmmm. Prototypes would be nice. If checking imports is complicated we could always drop it on the floor.

I have gotten the impression that content sniffing is considered harmful by a lot of web folks. (Implementation complexity and security issues arising from content confusion.) Polymorphic behavior based on URL content (JS or WASM) may be problematic unless we can require correct MIME types from the server 100% of the time. I’ll talk to the web folks on this side of the fence who have voiced concerns before. Content sniffing may also interact strangely with the “format layering” idea we’re kicking around for file format.

It will always possible to wrap a WASM instance in a ES6 module (manually or autogen) to prototype all of this without needed to change the loader. Does ES6 module integration need to be part of MVP? (I don't have strong feelings, it just noticed it seems separable.)

Nits / related comments:

  • Modules may include a relocation table, depending on how function pointers and/or dynamic linking are implemented. Not critical, but if we’re listing what’s contained in a module...
  • We may not want to support sbrk. I am considering scenarios where the system may want to steal memory from the program (automatically marshaling calls to the main thread, for example) and the only way to do this is forgetting sbrk and using a mmap-like interface. Currently I’d rate this idea as “problematic” and it may not see the light of day, but FYI.
  • General comment for everyone, no need for you to address it in this PR: there’s a lot of abstract thinking going on, concrete examples / psedocode would help make things clearer.

@jfbastien
Copy link
Member

@ncbray: for sbrk see #285.

@lukewagner
Copy link
Member Author

This is the only way I found to view a complete diff:
https://github.com/WebAssembly/design/compare/better-explain-modules
but it isn’t linked to the PR

I've been naively assuming that this is what "Files Changed" is showing. Anyhow, for reasons given in #282, out-of-line comments are preferable for non-nit comments.

At first glance, WASM importing WASM being type checked is potentially contentious/complicated.

Thus far, people have been relieved to hear this proposed (e.g., @flagxor, iirc) and while it may seem asymmetric at first glance, if you consider that an import is probing the host environment not just for a name, but for a (name, signature) pair (and faulting when not found), then all this amounts to is that, for a JS import, the signature is ignored in the probe (b/c there is no concept of signature in JS) while for a wasm import, there is a signature and so a "type mismatch" is really just a failed lookup. I would expect builtin imports to work the same way (less special cases to implement), so that actually makes JS the odd one out. Do you think it'd be more clear to describe this design this way?

There are also crazier things I’ve been thinking about, such as allowing a signed/unsigned distinction
in imports so values could be fixed up before passing them to JS. How does that interact?

Great idea! I think it could be done independent of JS by saying that the set of types available to import/export signature declarations additionally include sint32/uint32. From wasm's pov, these are both just int32s, but the extra sign information could be used by whatever language was on the other side of the interface. Think it's worth mentioning in this PR?

I have gotten the impression that content sniffing is considered harmful by a lot of web folks.

I have too, but I think a lot of this is from situations that are 100x harrier (e.g., character encoding, quirks mode and images) because the sniffing only came about after the fact. However, in our case, we're starting with a pretty strict input requirement (invalid characters trigger hard syntax errors in JS) and a pretty clear a priori break which we can test for web compat extensively before deploying. So while I'm definitely interested to hear specific concerns, I'm optimistic that our case is easier than those before.

Does ES6 module integration need to be part of MVP? (I don't have strong feelings, it just noticed it
seems separable.)

One way or another, we have to define how a wasm module gets loaded and how it looks to JS. If we don't integrate with ES6 modules, we'll have to design something separate which I think will have worse ergonomics and lead to duplication (spec and impl) and accidental incongruencies. If ES6 modules were imposing significant costs on the wasm design, then we'd have to weigh the pros/cons, but so far, and as I've tried to demonstrate in this PR, I'm not seeing any warts imposed on wasm due to ES6 modules atm.

Modules may include a relocation table, depending on how function pointers and/or dynamic linking
are implemented. Not critical, but if we’re listing what’s contained in a module...

By representing references symbolically (via an index into a module-local table of functions/globals/imports) instead of with hard-coded addresses, I'm not sure what extra information is needed to do relocations when dynamic linking. Do you have anything in mind?

General comment for everyone, no need for you to address it in this PR: there’s a lot of abstract
thinking going on, concrete examples / psedocode would help make things clearer.

Agreed; definitely looking forward to the next steps of giving a more formal/executable definition to all of this. For now, though, I think it's valuable to have this in place to address a lot of the immediate confusion.

@qwertie
Copy link

qwertie commented Jul 29, 2015

@ncbray What about generalizing that idea of signed/unsigned distinction to a general type, like ... "pointer to tuple (int32, sint)" or any little piece of metadata? Wasm doesn't necessarily have to define a metadata format, and could just offer a place to put it, with optional binary matching on import - e.g. if the export says Square("sint32" int32 x) where "sint32" is the metadata, an import could require Square("sint32" int32) or just Square(int32) if matching the metadata is not needed / supported. Could be stored as an index into a string table.

there’s a lot of abstract thinking going on, concrete examples / psedocode would help make things clearer.

Agreed! In PRs, I would strongly advocate giving a "possible specific design" instead of leaving it up to the imagination.

@lukewagner
Copy link
Member Author

Alright, have lgtm from @jfbastien and I don't see any specific outstanding objections to merging. Happy to iterate further in future PRs.

lukewagner added a commit that referenced this pull request Aug 3, 2015
Consolidate explanation of modules into a new Modules.md and improve explanation
@lukewagner lukewagner merged commit b5d1641 into master Aug 3, 2015
@lukewagner lukewagner deleted the better-explain-modules branch August 3, 2015 23:40
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.

7 participants