search

tinygo-org / tinygo

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

reflect: Add Function Reflect Support #4578

Open elewis787 opened 2 weeks ago

elewis787 commented 2 weeks ago

This adds reflection support for functions.

The following methods have been implemented:

This helps get closer to supporting text/templating and is related to 2494

Looking for feedback! There are a few things we could do to optimize this but our focus was on getting something workable as a first pass.

aykevl commented 2 weeks ago

Before I take a closer look, does this get text/template to work? Asking since it's going to be quite difficult to implement function calling, much more than methods like NumIn. And I'd like to know that the extra binary size is going to be worth it.

elewis787 commented 2 weeks ago

This pr doesn't get text templating working fully but I believe it's possible.

I've been looking at the method set data and that's where I believe I could use some guidance to not ballon the size.

I also want to take a closer look at text templating to see how deep this goes - call, make func, and a few others seem like they will be harder to implement.

I am working on tracking down if the method set is in memory already or if it is dropped. At first glance it seems like it is prepended to the types.

I am happy to mark this as a draft and continue digging into the other required methods to get text templating working.

dgryski commented 2 weeks ago

I think even minimal text/template support requires being able to call functions, as that is how calling the template "standard library" works.

elewis787 commented 2 weeks ago

Yes it does. Would you like to see this PR attempt to address all required functions for text/templating? If so, I'd be happy to turn this into a draft so we have something to track, unless there are other concerns.

elewis787 commented 2 weeks ago

I spent a little time walking through how this is implemented in Go. Confirming that it will be involved to get this working.

Quick Summary:

I haven't jumped into the tiny-go runtime much to see how to access the underlying values for reflection but I can see how some of this could be doable.

Time permitting, I will work on a few things as a first pass and follow-up.

Let me know the best way to handle this PR. I would like a spot to track/raise blockers.

aykevl commented 2 weeks ago

I believe we can easily find a way to use the method set to fill in the implementations for Method, MethodByName.

This will massively blow up the binary size, because it means we can't do dead code elimination of methods of any object that end up in an interface (and therefore might end up in a reflect.Value). So I'm not sure we want to do this, and probably don't want it enabled by default. (This is one of those little features that make it very difficult for compilers to optimize Go code for size). I can't tell you how much it will increase binary size, but it will be a lot.

Plain old functions on the other hand would be fine, because it's already possible to type-assert them back to a regular function value and call them.

Do you know whether we only need functions or also need methods for text/template?

Call and MakeFunc, will be much more difficult. I have not spent enough time to see what could be used to allocate a function. In Golang, they use a runtime call to execute the function logic, converting the function into a func([]value)([]value).

This is going to be even more difficult in TinyGo, because we use the C calling convention (mostly). But it is possible. We'll likely need code for every architecture assigning parameters to the right registers (notoriously difficult on amd64 for example) and then some assembly code as a trampoline. Again: possible but not at all easy. (We could do a limited subset relatively easily though).

elewis787 commented 2 weeks ago

Text/Templating does need methods. Link to exec in go

I haven't looked enough into the increase, but to your earlier point, even the function support increases the size more than expected ( can be optimized further ).

Good call outs on the parameter assigning, I have started looking through how to do this but haven't dug too deep yet.

For context, I am aiming to use text/templating in wasm/wasip2. A few packages that are nice to have require text/templating or more holistic reflection: cobra/viper, urfave, and tqla.

I think it would be acceptable to hide this through a feature for different arch types if possible.

aykevl commented 2 weeks ago

I think it would be acceptable to hide this through a feature for different arch types if possible.

This will increase the maintenance load, so I'd rather not let this depend on the architecture.

Also, to be clear: adding support for Method and MethodByName will not just increase binary size for packages that use text/template but for all programs. I don't know how much, maybe it's just 10% or so or maybe it doubles or triples binary size.

elewis787 commented 1 week ago

Yep, understand the increase in binary size and the concern.

I am also on board to avoid the feature/build flag. Only called this out as an option to align with the comments that we may be able to add support for a subset of architectures.

What are your overall thoughts - Worth taking a shot at the remaining reflection implementation and understanding the impact on size - or not worth the time/effort?

It sounds like we are converging on full reflection support across architecture types or not worth the effort at this time.

I am good with either direction, but would love to see reflection and text/templating support.

eliasnaur commented 1 week ago

Also, to be clear: adding support for Method and MethodByName will not just increase binary size for packages that use text/template but for all programs.

Do you mean all programs, or can the cost be avoided if nothing calls Method nor MethodByName?

elewis787 commented 1 week ago

Also, to be clear: adding support for Method and MethodByName will not just increase binary size for packages that use text/template but for all programs.

Do you mean all programs, or can the cost be avoided if nothing calls Method nor MethodByName?

I believe it will be all programs. This information is currently being optimized out at comp time - we would need to include it so we can access a lot of the info required by packages like reflect.