tinygo-org / tinygo

Go compiler for small places. Microcontrollers, WebAssembly (WASM/WASI), and command-line tools. Based on LLVM.
https://tinygo.org
Other
14.81k stars 872 forks source link

Support compiling basic Vecty/Vugu examples to WASM #93

Closed johanbrandhorst closed 3 years ago

johanbrandhorst commented 5 years ago

This is just a tracking issue for any outstanding work required for us to compile a vecty-style web app into WASM with tinygo. I think this would be a fantastic milestone for this repo and the Go WASM community at large, so lets see what remains outstanding.

Would love to have @slimsag take a quick look at this if possible, but otherwise I think @bketelsen will know the best what is outstanding.

aykevl commented 5 years ago

Channels (not sure this is strictly required)

Should be doable. Perhaps not select or multiple goroutines sending/receiving on the same channel, but basic send/recv between two goroutines might not be a lot of work.

  • reflect (also not sure if it's required, but I assume it is)

That's a really big one. If it is required, please tell me which part is because reflect will probably be implemented in parts.

Oh and I seem to remember vecty also needs callback support in syscall/js? That's not yet implemented AFAIK.

johanbrandhorst commented 5 years ago

Updated topic with callback support, thanks!

bketelsen commented 5 years ago

i'm not going to have time to look or work on this for about 2 weeks, but I'd like to consider removing the reflect requirements from vecty. That might mean it's less vecty and more something new.

johanbrandhorst commented 5 years ago

Interesting, would definitely be worthwhile thinking about coming at this problem from both ends, designing a Go web framework with TinyGo compatibility from the start could get us where we want significantly faster. Do you have a repo for this new framework already?

slimsag commented 5 years ago

I'm out of town and then on vacation for a few weeks, so I likely won't be able to follow up on this until after the holidays some time.

FWIW though the list mentioned above looks correct to me. I am very interested in adding TinyGo support to Vecty, I was actually looking into how hard it would be a few weeks back but the main kicker was the lack of reflect + syscall/js callback support.

Vecty's usage of reflect is pretty minimal, but is required. It is used for a few things:

  1. Doing a shallow copy of a component type. Example: There is a Component interface whose underlying value is *MyComponent and we need to make a copy of that the struct value (not the pointer). This is so that Vecty can "save" the previous render of your component so that subsequent renders can say "what has changed since the last time I rendered?" (the vecty.RenderSkipper interface). This is done here: https://github.com/gopherjs/vecty/blob/master/dom.go#L930-L938
  2. Copying the struct fields from one component instance to another if they are tagged with vecty:"prop"; any time a parent component returns a new &MySubComponent{} instance, Vecty keeps a single persistent component instance instead of using the new pointer value you have created and returned in your render function. This is really useful for e.g. binding event listeners to a component and not having to think about "is this the old component instance, or the new one?"; we do this here: https://github.com/gopherjs/vecty/blob/master/dom.go#L941-L967
  3. Component type comparison: We have Component(&Foo{}) and Component(&Baz{}) and we don't know if they are the same type or not (Vecty is only aware of the Component interface, not the underlying concrete types); we do that here: https://github.com/gopherjs/vecty/blob/master/dom.go#L908-L912

All other uses of reflect are just for improved debugging around printing type names, but nothing critical.

johanbrandhorst commented 5 years ago

Thank you so much for chiming in @slimsag! There's no reason we can't try to achieve this with Vecty or whatever vecty-like thing we might be able to create. The requirements for vecty then are narrowed down to:

This is obviously something TinyGo wants to support eventually anyway, but the real goal of this issue is for a vecty-like framework to be possible to use with TinyGo, so it's still valuable for us consider a new framework written from scratch to avoid the use of reflect specifically, since I don't think we'll ever be able to remove the other two requirements.

slimsag commented 5 years ago

For sure. One thing I would mention is that Vecty certainly did not use reflection because it was convenient, but rather because the API would be abysmal otherwise. I'd be super happy to see a React-like framework that doesn't require reflection and could be fully featured.. but I just don't see it being possible without the API being really unpleasant. If anyone can prove me wrong Vecty will change 😃

bketelsen commented 5 years ago

Hearing that is a strong discouragement from even trying... I know far less about it than @slimsag, not much chance I can do better.

slimsag commented 5 years ago

Thought: One viable option to bypass the reflection requirement would be code generation. Every component would have auto-generated ShallowCopy, CopyProps, IsSameType(other Component) methods. This would kind of suck because you would need to go generate after adding any new component to your codebase, but it would bypass the reflection requirement and could be a special case behind a tinygo build tag.

Does tinygo have its own build tag?

johanbrandhorst commented 5 years ago

I think code generation is definitely a viable alternative. As long as we can make that easy enough to use. I suppose we would have a generator that checks for some comment or embedded interface or something like that and generate into the same directory? If it's as easy as running vectygen ./... it should be fine I think?

slimsag commented 5 years ago

Yeah, it would be something like that; it would find all components by looking for structs that embed vecty.Core, and then write mycomponent.gen.go with implementations for the methods I described in my last post. That would remove the reflection requirement as long as there is a tinygo build tag or something.

Ideally that would be an intermediate solution and the long term solution here would still be the reflection approach so that code generation is not needed, since code generation is not great.

aykevl commented 5 years ago

How much of these features (like reflect) would be necessary for a "proof of concept" hello world example? It would be nice to have some sort of absolute minimum to iterate on.

EDIT: oh and yes there is a tinygo build tag you can use.

slimsag commented 5 years ago

@aykevl Conclusively I think:

I am on holiday starting next week, I may try to hack something together here. I'll let you know if I do.

aykevl commented 5 years ago

I've started work on reflection support in #104. See testdata/reflect.go for what is supported.

bradleypeabody commented 5 years ago

Fwiw, Vugu has similar requirements. https://github.com/vugu/vugu/issues/39 .

I'm not sure if there are other places where reflect is used but certainly the ability to obtain reflect.Value and check the Kind, and the ability to iterate over struct fields - Vugu needs those in order to function properly. And otherwise it's maps, and syscall/js.

As mentioned in Slack, once I have some more of the core functionality of Vugu built out I'll see if I can help contribute on some of these points.

Is there any sense in creating another issue to track Vugu-related support items for TinyGo? Or is it better to just track it all here in this issue?

aykevl commented 5 years ago

I don't think it's useful to add another issue. However, sharing what you need exactly here is very useful, like you just did.

ability to obtain reflect.Value and check the Kind

This has been implemented

iterate over struct fields

This should still be implemented. Should not be a massive amount of work, but not trivial either. (Something about priorities and hours in a day).

maps

There is very limited support, it's barely usable in its current state. Any help here is appreciated, you can take a look at the map implementation in the runtime of the main Go compiler for ideas. Wikipedia also gives some background information.

syscall/js

Many functions are implemented, but to be broadly useful more functions need to be implemented. In particular, callback support (js.FuncOf). The blocker for callback support is better support for maps, as syscall/js uses maps in its implementation. What do you specifically need from syscall/js? Which functions/methods are absolutely necessary?

bradleypeabody commented 5 years ago

Thanks for the feedback Ayke, that makes sense.

On syscall/js, it's essentially:

While complex programs built with Vugu might have more requirements, the above should make a basic Vugu program compile and run.

Regarding map support, sounds good. I will try following the instructions here to see if I can build from source on my machine (Mac laptop) and go from there.

aykevl commented 5 years ago

Update: the latest release (v0.6.0) has added support for js.FuncOf and has improved support for maps.

slimsag commented 5 years ago

https://github.com/gopherjs/vecty/pull/232 has just been merged which implements WebAssembly support in Vecty using the standard Go compiler.

While this doesn't automatically give us TinyGo support (unfortunately), it does give a clear picture of how far away we are. To get compilation passing we currently need:

I'll see if I can create a branch of Vecty soon that at least compiles under TinyGo to see if there are any other issues.

aykevl commented 5 years ago

Great! Now we have something to work towards.

slimsag commented 5 years ago

I've created a WIP / hacked together branch of Vecty which passes type checking under TinyGo: https://github.com/gopherjs/vecty/pull/243

Unfortunately, it doesn't pass compilation yet due to this issue: https://github.com/tinygo-org/tinygo/issues/440

aykevl commented 4 years ago

Unfortunately, it doesn't pass compilation yet due to this issue: #440

This has been fixed in the dev branch.

aykevl commented 4 years ago

See #480 that adds support for struct types in reflect which implements NumField and Field. It doesn't yet add support for StructTag.Get but that should be easy to add (it's basically just string processing). reflect.New should also not be terribly difficult to add.

deadprogram commented 4 years ago

480 has been merged to dev branch, so please test and report back @johanbrandhorst @slimsag if you can.

Thanks!

aykevl commented 4 years ago

Looks like more work is needed, but we're getting close!

../dom.go:719:69: only strings, bools, ints, pointers or structs of bools/ints are supported as map keys, but got: interface{}
../dom.go:391:36: only strings, bools, ints, pointers or structs of bools/ints are supported as map keys, but got: interface{}
../dom.go:395:20: only strings, bools, ints, pointers or structs of bools/ints are supported as map keys, but got: interface{}
../dom.go:431:47: only strings, bools, ints, pointers or structs of bools/ints are supported as map keys, but got: interface{}
../dom.go:515:21: only strings, bools, ints, pointers or structs of bools/ints are supported as map keys, but got: interface{}
../dom.go:538:11: only strings, bools, ints, pointers or structs of bools/ints are supported as map keys, but got: interface{}
../dom.go:870:11: only strings, bools, ints, pointers or structs of bools/ints are supported as map keys, but got: interface{Context() *github.com/gopherjs/vecty.Core; Render() github.com/gopherjs/vecty.ComponentOrHTML; isComponentOrHTML(); isMarkupOrChild()}

Also, getting this required copying the StructTag type from the reflect package, which is perhaps more complicated than necessary. I'm considering doing that at compile time. That's not fully compatible with the regular reflect interface, but I hope it won't really harm any real use of it. I checked the standard library and checked most files that imported both "reflect" and had a .Tag field access. That were 15 files. Most of those field accesses were unrelated to reflect, but the few that were and that I checked, all had the .Tag.Get("...") pattern.

kr1sten0 commented 4 years ago

Hi @aykevl , sorry to bother you I have encountered similar issue in my project, may i ask is there any progress on the reflect.New function recently? ❤️

deadprogram commented 4 years ago

What is the status of this issue, can we close it now?

bradleypeabody commented 4 years ago

I realize this issue is a hodgepodge of different things, but I'd still like get this working for Vugu. The first thing I run into is lack of module support, and thus re-attempting compilation requires some careful dependency management and I just haven't gotten to that yet (maybe it's easer than I'm thinking - I haven't looked in detail in a while).

I will try to get to this over the weekend though and provide an exact set of points that are blocking Vugu compilation. Or if this issue is too random and someone wants to close it I can certainly just make a new one when I have the info if that's better.

johanbrandhorst commented 4 years ago

I think this is the right issue. I think, like Vugu, there's still a few outstanding things to making TinyGo work with Vecty. I haven't tried it recently, maybe @slimsag will be able to point out exactly what.

aykevl commented 4 years ago

reflect.New shouldn't be very difficult, but depends on #588 (which I should still update to add tests).

slimsag commented 4 years ago

Apologies for the delay on reporting back the current status of the Vecty branch here! I hope to retry this and report back this weekend or next.

Vecty won't require any module support since it doesn't make use of any external dependencies (except for e.g. in example/demo applications), but reflect.New would be needed for a fully-functioning implementation as previously mentioned.

bradleypeabody commented 4 years ago

I took another look at Vugu support and the most immediate things I ran into were:

I'll see if I can come up with any more specifics.

But also it's a tradeoff - both of the issues above could probably be worked around in the Vugu source with some build tags (e.g. carefully implementing a subset of the functionality I need for encoding/json; and map[interface{}]*Result could be split into two and a uintptr to match the keys up, e.g. map[uintptr]interface{} and map[uintptr]*Result, I'm pretty sure the refection support is good enough to extract and use those uintptrs from an interface{} that points to a user-defined struct - not the greatest but it would work).

The lack of an http client is also something Vugu users will miss, however again, net/http in wasm is just a wrapper around JS's fetch() so I bet a minimalistic wrapper could be made with just what is needed for that and obviate the need for import "net/http" to work.

I'm on the fence if this is the right point in time to just dive in and do all of these build-tag changes in Vugu to make it work with Tinygo. It's certainly more realistic now than it was earlier this year but it's unfortunately really difficult to estimate the scope of this task. That said, so far I haven't found anything that is a full deal-breaker, just things that require workarounds 🤞(Oddly enough, the latest version of Vugu does not require reflect.New since references to component types have now become compile-time instead of run-time.)

UPDATE: After more work on this I was able to avoid the need for encoding/json and a few other non-critical packages and it certainly got a lot closer. The next step is going to be a refactor to split out Vugu's code generation from the things needed at runtime - which is long overdue anyway, but requires some careful thought. Thus far still no deal-breakers though, which is actually very encouraging and exciting. I'm going to put more effort into this as it looks like there's a good chance this can be made to work. If I find anything that's a blocker I'll add it to this issue.

marwan-at-work commented 4 years ago

Is this the right place to check if all reflection work has been done for Vecty to be unblocked? :)

ethanfrey commented 4 years ago

Hi @bradleypeabody

I was excited to see tinygo has wasm support and got a small example working, then I started to build out my use case, which used json as an api transport between wasm and "the world", and discovered that compiling with "encoding/json" caused the compiler to blow up.

It seems like you had a similar issue. How did you "work around" lack of that? Another json library? Another data format? I am fine putting everything in eg. protobuf or whatever if that will allow me a way to pass complex structs between languages.

Or did you do this with callbacks?

(Note: I'm not actually using a browser runtime, but using a standalone rust (wasmi) runtime)

bradleypeabody commented 4 years ago

Hi @ethanfrey I've done a fair amount of work getting Vugu to compile under Tinygo - the latest on master does now compile but functionally it's broken because of some things that need alternate implementation. Json is one of those things. I actually created the vjson package for this purpose but haven't written the Tinygo part of it yet (see https://github.com/vugu/vjson/blob/master/vjson-tinygo.go). The json parsing itself should be trivial to get working in tinygo and I believe a number of other json parsers out there will work fine - it's the more advanced reflection stuff that Tinygo has trouble with. Vugu does need some basic json support in order to work but it's minimal (I think only map[string]interface{} and basic types) and I plan on adding that into vjson as soon as possible. Otherwise most of the data transfer that Vugu does internally is encoded manually; I ended up making a simple "instruction" format for this - but that's really a Vugu-specific thing. If you are trying to transfer data from your app to a server, JSON is a good choice. And probably a third party library will do the trick.

aykevl commented 4 years ago

Take a look at https://github.com/tinygo-org/tinygo/pull/856, which adds support for interfaces as map keys.

ethanfrey commented 4 years ago

Hi @bradleypeabody thank you for the answer. I actually think map[sting]interface{} would be harder without reflection than an actual struct with fixed types (as many types will only be known runtime with the map). I did look around for other json libs and found easyjson, which I will try out. I know this is not directly related to vugu, but I think we want some similar functionality. I will let you know if I get this to work.

bradleypeabody commented 4 years ago

@ethanfrey Thanks and sounds good - yes, please do let me know how that goes.

To shed a little more light on it - the thing about map[string]interface{} where each of the values is one of the primitive types makes it possible to implement without using reflect.New(), which would otherwise be required to be able to instanciate a struct.

marwan-at-work commented 4 years ago

any updates here? :)

bradleypeabody commented 4 years ago

Although not entirely a blocker, module support in Tinygo would be a big help for Vugu. Vugu supports re-building your application on the fly (upon page refresh and I also just added a websocket auto reloader so editing a vugu file refreshes the page automatically). These and other tools and features are only supported and tested in conjunction with modules.

My current plan is that when Tinygo has module support I will dive back in and try to get Vugu+Tinygo working. I saw this PR https://github.com/tinygo-org/tinygo/pull/941 but am not sure how close this is to being functional.

(Although even as I write this I'm coming up with ideas for workarounds, but yeah, modules would make things much simpler.)

deadprogram commented 4 years ago

Hi @bradleypeabody thank you for the feedback. Module support is an important feature, and we will likely return to it after the release that we are currently preparing for this week, which adds Go 1.14 and LLVM 10 support.

If you have a chance to try out the latest dev branch with some of your code, it would be greatly appreciated to see if any of your previous issues are now resolved.

Thanks!

bradleypeabody commented 4 years ago

@deadprogram understood and thanks for the info. There are some challenges involved on my side but this is definitely on my todo list and I'll get back to you as soon as I can. (I do have a workaround in mind to make things work without modules and I'll try that out and see how far I can get.)

bradleypeabody commented 4 years ago

FYI good progress is being made on this. With this issue https://github.com/tinygo-org/tinygo/issues/1091 now resolved enough to move forward, for Vugu the ball is back in my court to start testing and trying out more complex examples. But basic page rendering and event handling plus re-render loop (i.e. stuff shows up and you can click buttons that toggle things on and off) is working now and with executables that are around 250k.

johanbrandhorst commented 4 years ago

That is fantastic news Brad, congratulations to everyone involved!

bradleypeabody commented 4 years ago

Vugu+TinyGo support now has basic functionality along with an example and some documentation: https://www.vugu.org/doc/tinygo . A majority of the Vugu test suite also now runs both the default Go compiler and TinyGo. There are still some test cases that don't work, but most of them do and I think it's working well enough to start trying it out.

I'll update with anymore specific issues that arise. I know http client support (i.e. making net/http compile and having http.Get map to JS fetch()) is one that will come up soon. I plan on doing some experimentation soon to see how much effort that would take to resolve.

johanbrandhorst commented 4 years ago

Awesome news Brad, thank you so much for your contributions to this project!

aykevl commented 4 years ago

That sounds great!

I saw that https://www.vugu.org/doc/tinygo says that modules are not supported:

By default Docker is used to call TinyGo. You can use wc.NoDocker() to disable this and have Vugu call the local tinygo executable from your path. Note that for now this results in various additional temporary folder structures being created during compilation to get the GOPATH setup correctly and thus is usually slower. Once TinyGo supports Go Modules, this limitation should go away.

By now, TinyGo does in fact support modules: #941.

bradleypeabody commented 4 years ago

@aykevl Nice! I didn't realize that. I've updated the doc page text for accuracy

... Vugu should be updated soon to remove this limitation and utilize TinyGo's module support.

And I will try this out. If everything plays well together I can simplify a few things and I'll see if it makes sense to make the local (non-Docker) call approach be the default. I suspect as TinyGo gets better and better the advantages of the Docker approach will become fewer and fewer (and not be worth the added overhead).

ethanfrey commented 4 years ago

@bradleypeabody I took a look at Vugu and am quite impressed with how much you managed to get into Wasm, and compile with TinyGo. My respect.

I am the CTO of https://github.com/CosmWasm/cosmwasm , a blockchain smart contracting module (that can plug into many different blockchains). We run Wasm modules and currently only have a Rust compile path. After watching TinyGo (and your progress with it) for some time, we have decided to start trying to port our smart contract framework to Go, and support both languages (Most blockchains we integrate with are written in Go).

I started looking at your code, and would love to reuse some of it (like https://github.com/vugu/vjson) and ideally contribute back in some way if I can. Is there a better way to reach out to you than responding to this issue?

bradleypeabody commented 4 years ago

@ethanfrey Will reply separately.