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

Untangle, move and hide singleton. #109

Closed modulovalue closed 1 year ago

modulovalue commented 1 year ago

This moves the runtime singleton into the higher level API so we can hide it, and removes it from the unrelated lower level wasmer bindings where it isn't necessary.


I think I have a clean solution for #47 that will also allow us to test for wasmer related memory leaks and fix that one place that leaks memory where the import isn't being disposed of. @liamappelbe could you recommend me a workflow to submit multiple PRs that depend on eachother in a way that would make it easy for you to review them and allow us to move faster?

liamappelbe commented 1 year ago

Is the goal of this just to change the runtime singleton from unexported to actually private? I'm fine with a change like that if it's zero cost, but this is not zero cost. It's more plumbing and more unnecessary fields on various classes that just duplicate the value of the singleton.

If you feel strongly about making it fully private, I'd prefer to just make all the files in this package part files, so they can see all the private things.

modulovalue commented 1 year ago

Is the goal of this just to change the runtime singleton from unexported to actually private?

That's one of the benefits. The main goal here is to decouple the wasmer bindings from our wasm api which will give us more flexibility.


I'm fine with a change like that if it's zero cost, but this is not zero cost

I believe you are referring to zero runtime cost. In which case, I absolutely agree. However, there are no real zero cost abstractions. It is always a trade-off between runtime/compile-time/dev-'time' cost.

I claim that removing the singleton will make the wasmer bindings more maintainable and that it is a trade-off that will benefit us in the long term maintenance-wise. Do you agree? If yes, then I suppose you claim that trading minor runtime cost saving for more maintainable code is not worth it. Is this observation correct?

modulovalue commented 1 year ago

I have some more work on a separate branch. There you can see what I mean when I'm talking about decoupling the wasmer bindings from our wasm_api. Notice how the wasmer bindings do not depend on anything else from the repo.

liamappelbe commented 1 year ago

I believe you are referring to zero runtime cost. In which case, I absolutely agree. However, there are no real zero cost abstractions. It is always a trade-off between runtime/compile-time/dev-'time' cost.

I claim that removing the singleton will make the wasmer bindings more maintainable and that it is a trade-off that will benefit us in the long term maintenance-wise. Do you agree? If yes, then I suppose you claim that trading minor runtime cost saving for more maintainable code is not worth it. Is this observation correct?

I'm not too worried about the runtime cost in this case. I don't see how this is more maintainable though. The additional plumbing is a (admittedly small) maintenance burden, and I'm not sure what the benefit is supposed to be. You haven't removed the singleton, you've just moved it. And I don't think singletons are universally a bad thing either. They have their issues, but sometimes they're the right tool for the job (especially in Dart, where we get lazy initialization and thread safety for free).

I have some more work on a separate branch. There you can see what I mean when I'm talking about decoupling the wasmer bindings from our wasm_api. Notice how the wasmer bindings do not depend on anything else from the repo.

Ok, I can see that runtime.dart doesn't import anything from the rest of the package, but I still don't understand why that's a benefit.

modulovalue commented 1 year ago

but I still don't understand why that's a benefit.

It's a little hard to explain because the benefits are subtle, but provide us with a lot of freedom.

One downside of having the singleton is that it forces all users of the wasmer bindings to depend on a single dynamic library. While it shouldn't be needed, some users might want to go deeper and provide a custom wasmer runtime or even switch it out during runtime. A global singleton does not make that possible.

You haven't removed the singleton, you've just moved it.

If you consider the version that I linked in my previous comment: we have removed it from the wasmer bindings (The wasmer bindings do not depend on the WasmModule<->Wasmer adapter, where the singleton has been moved to).


I personally would like to experiment with many things regarding WASM. Changes like these will provide the freedom for people like me to easily do experiments without having to fork this repo.

There are other more abstract arguments to be be made against singletons. Having singletons destroys the encapsulation capabilities that classes are meant to provide. This, in turn, negatively affects referential transparency. A different argument: the notion of pure functions can be applied to classes because classes can be simulated using product types, and product types can be implemented using lambda calculus. Therefore, we also lose purity by having a global singleton.

liamappelbe commented 1 year ago

If someone has a use case we don't support, then we can look at the details of that use case and figure out a solution. Until then I'm not inclined to add additional complexity to allow hypothetical users to swap out parts of the package. That route ends up with layers and layers of classes that are just plumbing for other classes. It makes the code hard to read, which is a problem for maintainability.

Contrast that with your other changes, which have been good progress moving in the direction of allowing different implementations of the public API. This has the immediate practical benefit of allowing JS support (which has been on my TODO list for a long time).

modulovalue commented 1 year ago

That route ends up with layers and layers of classes that are just plumbing for other classes. It makes the code hard to read

I'm sorry if the layers that I'm trying to add came across as arbitrary. I agree, that adding layers for the sake of being modular is in general not good. However, I see a very good separation that emerges here that I think we should not suppress:

  1. wasm_api will be our public api that users will depend on, and will be covered by semantic versioning. (done)
  2. js bindings that do not depend on wasm_api, that users can use if they want to, but will most likely not be covered by semantic versioning. (in progress)
  3. wasmer bindings that do not depend on wasm_api, that users can use if they want to, but will most likely not be covered by semantic versioning. (close to done)
  4. an adapter between wasm_api <-> js bindings so that we can hide the js internals from users and only expose wasm_api. (in progress)
  5. an adapter between wasm_api <-> wasmer bindings so that we can hide the wasmer internals from users and only expose wasm_api. (done)

I think that 2 and 3 are necessary because js-wasm and wasmer are not 1:1 congruent with each other:

Furthermore, WASM is an evolving standard, and wasmer is built by a for profit company that has completely different incentives when compared to the web, where stability is priority number 1. Users, and I, will want to have access to wasmers cutting edge features e.g. see #66 and not be limited by the lowest common denominator, which is the web.

My experience tells me that having a singleton here is an absolutely bad idea. I've been in this same position many times, and I am very certain that my previous experience applies here as well. If you disagree with me, plenty of people have already written tons about why singletons are bad.

(I'm not saying that you're wrong, yes, I agree, in some cases singletons are helpful.)

liamappelbe commented 1 year ago
  1. wasm_api will be our public api that users will depend on, and will be covered by semantic versioning. (done)
  2. js bindings that do not depend on wasm_api, that users can use if they want to, but will most likely not be covered by semantic versioning. (in progress)
  3. wasmer bindings that do not depend on wasm_api, that users can use if they want to, but will most likely not be covered by semantic versioning. (close to done)
  4. an adapter between wasm_api <-> js bindings so that we can hide the js internals from users and only expose wasm_api. (in progress)
  5. an adapter between wasm_api <-> wasmer bindings so that we can hide the wasmer internals from users and only expose wasm_api. (done)

That sounds like a good design to me. But I don't really understand what that has to do with moving the singleton from runtime.dart (the wasmer bindings in point 3) to module.dart (the adapter in point 5)? That's the part that leads to all the extra plumbing. Enabling users to directly access wasmer sounds fine to me, but that's totally doable regardless of whether the singleton lives in runtime.dart or module.dart.

modulovalue commented 1 year ago

@liamappelbe Please take a look at the following library dependency graphs:

Where I want us to be: link The current state: link

But I don't really understand what that has to do with moving the singleton from runtime.dart (the wasmer bindings in point 3) to module.dart

Notice how (now) runtime.dart depends on wasmer_locator and wasm_api, but it shouldn't if we want to be maintainable. Cutting these unnecessary dependencies makes it much easier to reason about code. We will have a non trivial amount of glue code (js, wasmer, wasm, js-to-wasm, wasmer-to-wasm) it will only benefit us to keep our package-internal dependency graph shallow. And moving that singleton up to the module, out of the runtime, helps us do that. (I hope this makes it more clear why the singleton there is suboptimal. Let me know if it doesn't.)

liamappelbe commented 1 year ago

Ok, sounds like you have a solid plan in mind.