-
Notifications
You must be signed in to change notification settings - Fork 25
Conversation
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. |
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 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 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. |
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).
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. |
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.
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. |
Use the singleton less.
4569be4
to
eba9e23
Compare
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). |
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:
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. (I'm not saying that you're wrong, yes, I agree, in some cases singletons are helpful.) |
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. |
@@ -412,22 +425,35 @@ class WasmImportDescriptor { | |||
final String moduleName; | |||
final String name; | |||
final Pointer type; | |||
|
|||
WasmImportDescriptor._(this.kind, this.moduleName, this.name, this.type); | |||
final String description; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels weird to have this as a field since it's only for debugging. I'd rather just have a reference to the runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
I've started to work on generating bindings to wasm and our exposed export and import structures will need some adjustment, I'll clean that up in a separate PR if that's fine with you.
@liamappelbe Please take a look at the following library dependency graphs: Where I want us to be: link
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.) |
Ok, sounds like you have a solid plan in mind. |
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?