Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reflect: Add Function Reflect Support #4578

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

elewis787
Copy link

@elewis787 elewis787 commented Nov 1, 2024

This adds reflection support for functions.

The following methods have been implemented:

  • IsVariadic
  • In
  • Out
  • NumIn
  • NumOut

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

aykevl commented Nov 2, 2024

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

elewis787 commented Nov 2, 2024

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

dgryski commented Nov 3, 2024

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

@elewis787
Copy link
Author

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

elewis787 commented Nov 4, 2024

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 believe we can easily find a way to use the method set to fill in the implementations for Method, MethodByName.
  • 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).

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

aykevl commented Nov 7, 2024

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

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

aykevl commented Nov 8, 2024

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

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

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

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 in so we can access a lot of the info in packages like reflect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants