Skip to content

proposal: reflect: add alloc-free way retrieve value from Value #46093

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

Closed
josharian opened this issue May 10, 2021 · 13 comments
Closed

proposal: reflect: add alloc-free way retrieve value from Value #46093

josharian opened this issue May 10, 2021 · 13 comments

Comments

@josharian
Copy link
Contributor

It is currently impossible to retrieve large values (e.g. [32]byte) from a reflect.Value without allocating. There are specialized getters for common types (Value.Int, Value.Bytes, etc.), but nothing general purpose.

I propose that we add a new API that lets the caller bring their own storage. Something like this:

// Store stores the value in v into *x.
// x must have type *T, where T is v's value's type.
func (v Value) Store(x interface{})

Then the caller can do:

v := reflect.ValueOf([32]byte{/*... */})
var a [32]byte
v.Store(&a)

This call pattern might not work out of the gate, since the argument to Value.Store will escape. We might be able to fix that with appropriate annotations and/or toolchain support.

But even if not, the caller could use a sync.Pool of *[32]byte. The key thing is that the allocation is moved to the caller's side.

We'd need a Value.CanStore too, unless we are comfortable re-using Value.CanInterface for it.

@gopherbot gopherbot added this to the Proposal milestone May 10, 2021
@randall77
Copy link
Contributor

Can't you do:

    var v reflect.Value = ...
    var a [32]byte
    reflect.ValueOf(&a).Elem().Set(v)

@josharian
Copy link
Contributor Author

Hmmmm. In toy benchmarks, yes. In my larger benchmark, no. Possibly user error; I'll report back.

@josharian
Copy link
Contributor Author

Ah. With the reflect.ValueOf.Elem.Set approach, I'm ending up with a escaping to the heap. If I could use a *[32]byte, as I could with Value.Store, then I could make a sync.Pool of them, but that's not possible. (Or if it were single-threaded, I could use a global a.)

@randall77
Copy link
Contributor

Why can't you make a sync.Pool of *[32]bytes?

Related: #43732 #32424

@josharian
Copy link
Contributor Author

josharian commented May 11, 2021

This panics:

var v reflect.Value = ...
a :=  new([32]byte)
reflect.ValueOf(a).Set(v)

I may be holding it wrong, but I don't see how to use .Set with a *[32]byte instead of a [32]byte.

@randall77
Copy link
Contributor

package main

import (
	"fmt"
	"reflect"
)

func main() {
	v := reflect.ValueOf([32]byte{1, 2, 3, 4, 5})
	a := new([32]byte)
	reflect.ValueOf(a).Elem().Set(v)
	fmt.Printf("%v\n", *a)
}

Seems to work.

@josharian
Copy link
Contributor Author

Thanks, Keith. I'm experimenting a bit here still. I'll report back.

@rsc
Copy link
Contributor

rsc commented May 19, 2021

Is this a duplicate of #46131 now?

@rsc
Copy link
Contributor

rsc commented May 19, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@bradfitz
Copy link
Contributor

I don't think they're dups. But I'm curious what problem @josharian hit with @randall77's latest comment.

@josharian
Copy link
Contributor Author

Something non-obvious happening. Still in my queue. Thought was going to be today, probably tomorrow.

@josharian
Copy link
Contributor Author

I wasn't taking sufficient care that my values didn't escape. I finally got this working as anticipated. Thanks, Keith, and sorry about the noise.

@rsc
Copy link
Contributor

rsc commented May 26, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@golang golang locked and limited conversation to collaborators May 26, 2022
@rsc rsc moved this to Declined in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@rsc rsc removed this from Proposals Oct 19, 2022
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

5 participants