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

proposal: reflect: rename ArenaNew as NewFrom #56234

Closed
dsnet opened this issue Oct 14, 2022 · 5 comments
Closed

proposal: reflect: rename ArenaNew as NewFrom #56234

dsnet opened this issue Oct 14, 2022 · 5 comments

Comments

@dsnet
Copy link
Member

dsnet commented Oct 14, 2022

CL 423361 adds reflect.ArenaNew to allocate a new Value from an arena.

As a form of bikeshedding, I think the "Arena" prefix is unfortunate since it means related functionality will not sort well in godoc. Perhaps we should establish a convention for a suffix that indicates the use of an *arena.Arena?

I propose using From as a suffix to indicate that the allocation was made from an allocator:

func New(Type) Value
func NewAt(Type, unsafe.Pointer)
func NewFrom(Type, *arena.Arena) Value

as it would coexist nicely with the New function with an At suffix.

You can imagine other related functionality added in the future:

MakeSlice(Type, int, int) Value
MakeSliceFrom(Type, int, int, *arena.Arena) Value

MakeMap(Type) Value
MakeMapFrom(Type, *arena.Arena) Value

MakeMapWithSize(Type, int) Value
MakeMapWithSizeFrom(Type, int, *arena.Arena) Value

MakeChan(Type, int) Value
MakeChanFrom(Type, int, *arena.Arena) Value

MakeSliceFrom needs to allocate the slice header (not just the backing array) somewhere. I believe the correct behavior is to also allocate the header from the arena.

With the acceptance of #48000, we could also add:

func (Value) Grow(int)
func (Value) GrowFrom(int, *arena.Arena)

Notably missing from the list is the arena variants of Append and AppendSlice. If we put the arena.Arena argument at the end, it would not work well with Append, which has a variadic argument. Also, the From suffix seems ambiguous whether it is referring to the arena or the source slice. I think this omission is okay since the implementations for those two functions are relatively easy to implement yourself given the existence of Value.GrowFrom.

Other considerations: Having these functions take in an *arena.Arena make it such that supporting a different allocator in the future would require expanding the API surface again. Is there an interface we could accept? Would there ever be a different allocator implementation?

\cc @danscales @mknyszek @cherrymui

@dsnet dsnet added the Proposal label Oct 14, 2022
@gopherbot gopherbot added this to the Proposal milestone Oct 14, 2022
@mknyszek
Copy link
Contributor

I'm personally not a huge fan of "From" because it feels too generic of a namespace to take for arenas, especially given this scenario:

Is there an interface we could accept? Would there ever be a different allocator implementation?

Such an interface would only ever come after multiple such allocators exist, I think. I also don't think there are any other ideas currently being seriously considered for memory allocators.

What about a suffix like "InArena"? It's a bit verbose, but it gets the point across.

@dsnet
Copy link
Member Author

dsnet commented Oct 14, 2022

Alternatively, we could try and go for an allocator interface:

type Allocator interface {
    New(p any)                 // where p is a **T
    MakeSlice(p any, int, int) // where p is a *[]T
    GrowSlice(p any, int)      // where p is a *[]T
    MakeMap(p any, int)        // where p is a *map[K]V
    MakeChan(p any, int)       // where p is a *chan T
}

However, I'm not sure how much performance cost will be lost going through a virtual method call in the interface.

@dsnet
Copy link
Member Author

dsnet commented Oct 14, 2022

Actually, if we declared those methods on arena.Arena, I'm not sure we need to expand the "reflect" API surface at all, since any usage of "reflect" could just use those instead.

For example, instead of:

va.Set(reflect.NewFrom(va.Type().Elem(), a))

we could just do:

a.New(va.Addr().Interface())

Instead of:

va.Set(reflect.MakeMapFrom(t, a))

we could just do:

a.MakeMap(va.Addr().Interface(), 0)

@rsc
Copy link
Contributor

rsc commented Oct 14, 2022

Given that #51317 has not been accepted and this API is not in any published Go release, nor is it planned to be any time soon, I don't think we need to fine-tune the names of the symbols the experiment defines. Closing this as a duplicate of #51317.

@rsc rsc closed this as not planned Won't fix, can't repro, duplicate, stale Oct 14, 2022
@mknyszek
Copy link
Contributor

I understood that this issue was about an extension to the arena proposal, rather than an extension to the GOEXPERIMENT. Arguably this could be posted on the original issue, but that issue is still locked, I think. (I'm not sure if @dsnet is able to reply there.)

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

No branches or pull requests

4 participants