Skip to content
New issue

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

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

Already on GitHub? # to your account

Split out compileAndInstantiate from instantiate #999

Closed
wants to merge 1 commit into from

Conversation

lukewagner
Copy link
Member

I think there would be a number of benefits from having instantiate always return a Promise<Instance> and having a separate compileAndInstantiate that returns the Promise<{module,instance}>. In this PR, instantiate is still able to take all the same things it does now, and thus, this PR is strictly more expressive than the current state. (It's also a pretty simple change to implement.)

The benefits I see of this PR are:

  • instantiate is less surprising; compileAndInstantiate returning a pair is implied by the name.
  • It's easier to teach wasm if instantiate is just a sink for all manner of byte-y things that always produces an Instance. I was feeling this acutely while writing MDN docs/tutorials recently.
  • With instantiate, the engine would know up front that the Module won't be needed and that the generated code can't be instantiated multiple times, serialized or shared. This can allow for more efficient compilation, more aggressive specialization and less garbage generated by compilation.
  • Resolves awkward design question for instantiateGroup (Support multi-module 1-step Instantiate with dependencies #997): given that this function takes a map of inputs and produces a map of outputs: are the output values Instances or {module,instance} pairs? Do we have it depend on the dynamic types of values in the input map? What if some of the input values are Modules and some are not? Do we instead break symmetry and have it always return a {module,instance} pair? After this PR, the obvious, symmetric thing would be to have both instantiateGroup and compileAndInstantiateGroup which makes the output type clear.

@lukewagner
Copy link
Member Author

Sadly, it's too late to make such a breaking change, sorry for the noise. This would've been nice to have, but it's not necessary.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant