Skip to content
This repository has been archived by the owner on Jul 1, 2023. It is now read-only.

Untangle, move and hide singleton. #109

Merged
merged 1 commit into from
Nov 22, 2022

Conversation

modulovalue
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

Use the singleton less.
@liamappelbe
Copy link
Contributor

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
Copy link
Contributor Author

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:

  • I believe, js-wasm does not support synchronous compilation? (but wasmer does, and it makes sense to provide synchronous compilation, hence 3.)
  • I believe, js-wasm does not support WASI? (but wasmer does, and it makes sense to provide the features enabled by WASI to users that don't need js)

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
Copy link
Contributor

  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.

@@ -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;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@modulovalue
Copy link
Contributor Author

@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
Copy link
Contributor

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

@liamappelbe liamappelbe merged commit 8b7ffb6 into dart-archive:main Nov 22, 2022
@modulovalue modulovalue deleted the untangle_singleton branch November 22, 2022 22:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants