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

cmd/compile: optimize reflect.Value.Set with short-lived Values #57060

Open
dsnet opened this issue Dec 2, 2022 · 3 comments
Open

cmd/compile: optimize reflect.Value.Set with short-lived Values #57060

dsnet opened this issue Dec 2, 2022 · 3 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Dec 2, 2022

Some accessor reflect.Value methods must allocate since they return value that references an underlying value that must be heap allocated.

Such patterns include:

v1.Set(v2.MapIndex(...))
v1.Set(v2.Slice(...))
v1.Set(v2.Slice3(...))

In such a usage, the generated value could be stack allocated since it only used as an intermediate storage before the value is stored into v1.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 2, 2022
@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 5, 2022
@prattmic
Copy link
Member

prattmic commented Dec 5, 2022

cc @golang/compiler

@cherrymui
Copy link
Member

What could be stack allocated? You mean the Value struct? Or the underlying data, like the slice header for the v2.Slice case? Thanks.

@dsnet
Copy link
Member Author

dsnet commented Dec 5, 2022

In the case of Value.MapIndex, it would be the final call to copyVal:

return copyVal(typ, fl, e)

Suppose we refactored to Value.MapIndex to be inlinable, where the call to copyVal was part of the inlined portion, I wonder if the compiler can detect that the returned Value does not escape and cause the call to unsafe_New in copyVal to be stack allocated:

c := unsafe_New(typ)

I'm not sure if that's possible though, since the size of a stack allocated object usually needs to be known at compile time. In this situation we don't know.

The alternative is to avoid copyVal entirely if the compiler detects that the Value is directly passed to Value.Set. It's okay if we temporarily pass a value that addresses a map value if we know that it is short-lived. However, this seems like a fairly specific compiler hack that depends on particular ways that reflect is implemented today.

@seankhliao seankhliao modified the milestone: Backlog Dec 8, 2022
@prattmic prattmic added this to the Backlog milestone Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
Development

No branches or pull requests

5 participants