Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

reflect: Add Function Reflect Support#4578

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
elewis787 wants to merge3 commits intotinygo-org:dev
base:dev
Choose a base branch
Loading
fromelewis787:elewis-function-reflect

Conversation

elewis787
Copy link

@elewis787elewis787 commentedNov 1, 2024
edited
Loading

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 to2494

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

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 reacted with thumbs up emoji

@elewis787
Copy link
Author

elewis787 commentedNov 2, 2024
edited
Loading

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

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

elewis787 reacted with thumbs up emoji

@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 commentedNov 4, 2024
edited
Loading

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 afunc([]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

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 areflect.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 youhow 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 afunc([]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 reacted with thumbs up emoji

@elewis787
Copy link
Author

Text/Templating does need methods. Link to exec ingo

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, andtqla.

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

@aykevl
Copy link
Member

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 forMethod andMethodByName will not just increase binary size for packages that use text/template but forall 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 forMethod andMethodByName will not just increase binary size for packages that use text/template but forall programs.

Do you mean all programs, or can the cost be avoided if nothing callsMethod norMethodByName?

@elewis787
Copy link
Author

elewis787 commentedNov 11, 2024
edited
Loading

Also, to be clear: adding support forMethod andMethodByName will not just increase binary size for packages that use text/template but forall programs.

Do you mean all programs, or can the cost be avoided if nothing callsMethod norMethodByName?

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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@elewis787@aykevl@dgryski@eliasnaur@jeff1010322

[8]ページ先頭

©2009-2025 Movatter.jp