dart-archive / wasm

Utilities for loading and running WASM modules from Dart code
https://pub.dev/packages/wasm
BSD 3-Clause "New" or "Revised" License
312 stars 24 forks source link

Independent wasmer bindings #112

Closed modulovalue closed 1 year ago

modulovalue commented 1 year ago

This PR depends on #109 The purpose of this PR is to streamline the wasmer bindings, this includes:

@liamappelbe I've decided to submit this PR to present to you why it would be beneficial for us to get rid of the singleton, as this wouldn't really be possible because we'd still have to somehow depend on wasm_locator (inside of runtime.dart) as long as we need a singleton.

modulovalue commented 1 year ago

@liamappelbe I do intend for this to land (it's not just an example, perhaps I was a little unclear about that).

liamappelbe commented 1 year ago

The DI changes are fine. Not sure why you want to merge all those files though.

The runtime/wasmer_api split is for readability. I find it easier to work with multiple small files than one big file, as long as the division is sensible. Here the split is that wasmer_api contains things that directly mirror (or thinly wrap) elements of the Wasmer C API, whereas runtime adds Dart specific logic (eg memory management) that makes the Wasmer C API more ergonomic.

I also like having the templates as standalone files, because it makes it easier to see where the generated files came from.

liamappelbe commented 1 year ago

If you add me as a reviewer, it's more likely I'll see the PR and do a proper code review.

modulovalue commented 1 year ago

If you add me as a reviewer

I don't think github allows me to do that. Probably because I don't have the permissions to do that.


Not sure why you want to merge all those files though.

In general, people tend to create files based on what they think should belong in a new file, which is highly subjective. I have a very objective rule for what should not belong in a new file: anything that can be contracted without affecting the underlying dependency graph.

Consider the dependency graphs that I posted in a previous PR. This PR brings us here: link And this is where we are right now: link

Notice how currently module depends on wasmer_api. That dependency is transitive because module also depends on runtime which also depends on wasmer_api. That means that we can contract the edge between runtime<->wasmer_api without changing the underlying dependency graph. Files that can be contracted should in my opinion be contracted and in my experience, this does help immensely with maintainability as it makes navigating a codebase much easier because we can rely on certain invariants to hold.

Here the split is that wasmer_api contains things that directly mirror (or thinly wrap) elements of the Wasmer C API,

But that's not the case today, because bindings are being generated not just into wasmer_api but also into runtime. To fully implement that idea we'd have to move all the pure Wasmer related bindings into its own file and make sure that we don't have module depend on the pure Wasmer bindings (i.e. don't have a transitive dependency there as we have today.). (I'm not against that, but since we don't plan to expose the pure wasmer C bindings, I don't see that as absolutely necessary)

The contraction argument applies to python as well. Also, here are all the questions that I asked myself about the template files that get automatically answered by inlining them into the python file:

I hope you can see how the answers to these questions are not obvious to new maintainers. We can answer them implicitly, and therefore reduce complexity, by inlining the templates into the python file.