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: permit Value.Bytes (but not SetBytes) on addressable byte arrays #47066

Closed
bradfitz opened this issue Jul 6, 2021 · 23 comments
Closed

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Jul 6, 2021

I'm trying to get a []byte of an array via reflect, without allocations.

reflect.Value.Bytes says:

Bytes returns v's underlying value. It panics if v's underlying value is not a slice of bytes.

I have an array, not a slice. I can slice it, but reflect.Value.Slice call allocates:

func TestArraySliceAllocs(t *testing.T) {
        type T struct {
                X [32]byte
        }
        x := &T{X: [32]byte{1: 1, 2: 2, 3: 3, 4: 4}}

        var b []byte
        n := int(testing.AllocsPerRun(2000, func() {
                v := reflect.ValueOf(x)
                b = v.Elem().Field(0).Slice(0, 5).Bytes()
        }))
        if n != 0 {
                t.Errorf("allocs = %d; want 0", n)
        }
        const want = "\x00\x01\x02\x03\x04"
        if string(b) != want {
                t.Errorf("got %q; want %q", b, want)
        }
}

(fails with 1 alloc, from Slice)

Perhaps reflect.Value.Bytes could also permit getting a []byte of an array of bytes?

Or maybe I'm holding reflect wrong and there's an alloc-free way to do this already.

/cc @josharian

@randall77
Copy link
Contributor

If you have a pointer to an array of bytes in a reflect.Value, you could do

p := v.Interface().(*[8]byte)[:]

(assuming you know 8)

In your code, that's b = v.Elem().Field(0).Addr().Interface().(*[32]byte)[:]

@bradfitz
Copy link
Contributor Author

bradfitz commented Jul 6, 2021

I don't know the length, though. I want to do this in a package that can run over anybody's types at runtime.

@randall77
Copy link
Contributor

randall77 commented Jul 6, 2021

I see. I think it would be fine to allow Bytes to work on an addressable array (or pointer to array?).

@josharian
Copy link
Contributor

josharian commented Jul 6, 2021

Is it possible to make Slice not allocate when the array is addressable (or when it is a pointer to an array)?

@DeedleFake
Copy link

DeedleFake commented Jul 6, 2021

Maybe I'm more tired than I think and I'm completely misreading something here, but doesn't that test code make sure that Slice() doesn't allocate? It marks it as an error if testing.AllocsPerRun() returns anything other than zero. The error message even explicitly says allocs = %d; want 0.

Edit: I'm definitely more tired than I think. Somehow completely missed the

(fails with 1 alloc, from Slice)

underneath.

I read through both reflect.Value.Slice() and reflect.Value.Bytes(). I think special-casing Bytes() to allow calling it with an array makes sense. I'd prefer being able to get rid of the allocation in Slice(), but I'm not sure that that's particularly feasible without changing the structure of reflect.Value. Am I correct that the allocation happens because reflect.Value can only hold at most a word of data, so it stores a pointer to the newly created slice header instead, which causes it to escape to the heap on line 1810? -gcflags='-m' doesn't seem to do anything about stdlib packages, even if I combine it with -a.

It should also probably be noted that this is pretty easy to workaround, especially in Go 1.17:

package main

import (
	"fmt"
	"reflect"
	"testing"
	"unsafe"
)

func main() {
	v := reflect.ValueOf(&[...]byte{3, 5, 7}).Elem()
	var s []byte
	n := testing.AllocsPerRun(1024, func() {
		s = unsafe.Slice((*byte)(unsafe.Pointer(v.Addr().Pointer())), v.Len())
	})
	fmt.Println(n)
}

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jul 14, 2021
@dsnet
Copy link
Member

dsnet commented Sep 4, 2021

I like @randall77's suggestion of allow Bytes on an addressable array or non-nil pointer to array.

In some code, I have:

if t.Kind() == reflect.Array {
	b = va.Slice(0, t.Len()).Bytes()
} else {
	b = va.Bytes()
}

I would like to reduce it to simply:

b = va.Bytes()

and avoid the allocation that occurs in Slice.

@dsnet
Copy link
Member

dsnet commented Sep 4, 2021

Analyzing the module proxy, I found:

  • 655 cases of x.Slice(...).Bytes()
  • 648 cases of x.Slice(0, ...).Bytes() (98.9% of total)
  • 276 cases of x.Slice(0, x.Len()).Bytes() (42.1% of total)

Of the 372 cases of x.Slice(0, ...).Bytes() that aren't x.Slice(0, x.Len()).Bytes(), many were like x.Slice(0, n).Bytes(), where n was the length of the array, but obtained elsewhere to avoid repeatedly calling reflect.Value.Len.

@rsc rsc changed the title proposal: reflect: permit Value.Bytes on byte arrays? proposal: reflect: permit Value.Bytes on addressable byte arrays Oct 13, 2021
@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

Making v.Bytes() work when v is an addressable byte array seems OK.
Or can we optimize away the slice conversion in v.Slice().Bytes()?

@rsc
Copy link
Contributor

rsc commented Oct 13, 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

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 13, 2021
@randall77
Copy link
Contributor

Optimizing the allocation out of v.Slice() sounds hard. The tricky part is to keep track of the args to Slice somewhere.
We could conceivably introduce another flag to reflect.Value.flag which means "this is a slice of a ptr to an array with slice bounds 0 and len(v.Type.Elem())", and set that flag when we're slicing a pointer-to-array with the right Slice args. Then everywhere that accepts a slice type needs a special case to recognize this flag.

All to avoid just letting Bytes work on addressable arrays, that seems overkill.

@dsnet
Copy link
Member

dsnet commented Oct 13, 2021

We could conceivably introduce another flag to reflect.Value.flag which means "this is a slice of a ptr to an array with slice bounds 0 and len(v.Type.Elem())"

This would unfortunately still not help the common patterns where reflect.Value.Slice and reflect.Append (#48000) always allocate.

@dsnet
Copy link
Member

dsnet commented Oct 14, 2021

Making v.Bytes() work when v is an addressable byte array seems OK.

In addition, we should probably update Value.SetBytes should stay consistent with Value.Bytes.

@rsc
Copy link
Contributor

rsc commented Oct 20, 2021

SetBytes seems more difficult.
What happens if you v.SetBytes(x) and v.Len() and len(x) are different sizes?

@gopherbot
Copy link

Change https://golang.org/cl/357331 mentions this issue: reflect: allow Value.Bytes and Value.SetBytes on byte arrays

@dsnet
Copy link
Member

dsnet commented Oct 20, 2021

I mailed out https://golang.org/cl/357331 as a prototype.

What happens if you v.SetBytes(x) and v.Len() and len(x) are different sizes?

I would expect it to panic. The Bytes method already makes implicit assumptions about the length of the []byte that it returns for an array. It seems reasonable for SetBytes to have expectations on the length it accepts.

@randall77
Copy link
Contributor

That SetBytes behavior is subtly different though. SetBytes on a []byte just replaces pointers, it does no copying. Whereas SetBytes on an array does a copy of the underlying data.

Unlike Bytes, where the returned []byte is always aliased to the storage for the input slice or array.

Would it make more sense for SetBytes on an addressable *[N]byte to just update the pointer to point to the first N bytes of the input byte slice?

That means you couldn't SetBytes on an addressable [N]byte. You would use Copy for that (which is the behavior your prototype does, I think).

@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

That is in fact subtle, @randall77.
It makes me think we should leave SetBytes as is, maybe?
It looks like today SetBytes requires a slice.
It seems safest to just keep it that way.

@randall77
Copy link
Contributor

randall77 commented Oct 27, 2021

Agreed. If you really want the alias behavior, you can use Set, and if you really want the copy behavior, you can use Copy.

var a *[4]byte = ...
var b *[4]byte = ...

// copy b to a
va := reflect.ValueOf(a)
vb := reflect.ValueOf(b)
reflect.Copy(va.Elem(), vb.Elem())

// alias b to a
va := reflect.ValueOf(a)
vb := reflect.ValueOf(&b).Elem()
vb.Set(va)

Unlike with []byte, array pointers can be put in interfaces (and reflect.Values) without allocation, so having a separate Set function is less useful.

@dsnet
Copy link
Member

dsnet commented Oct 28, 2021

@randall77 Good point about subtly different behavior for []byte with regard to SetBytes.

It feels weird that Bytes and SetBytes are asymmetrical, but maybe that's a necessity.

@rsc
Copy link
Contributor

rsc commented Nov 3, 2021

Indeed - reading and writing are asymmetrical.

@rsc rsc changed the title proposal: reflect: permit Value.Bytes on addressable byte arrays proposal: reflect: permit Value.Bytes (but not SetBytes) on addressable byte arrays Nov 3, 2021
@rsc rsc moved this from Active to Likely Accept in Proposals (old) Nov 3, 2021
@rsc
Copy link
Contributor

rsc commented Nov 3, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Nov 10, 2021
@rsc
Copy link
Contributor

rsc commented Nov 10, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: reflect: permit Value.Bytes (but not SetBytes) on addressable byte arrays reflect: permit Value.Bytes (but not SetBytes) on addressable byte arrays Nov 10, 2021
@rsc rsc modified the milestones: Proposal, Backlog Nov 10, 2021
@dmitshur dmitshur modified the milestones: Backlog, Go1.19 Mar 4, 2022
@gopherbot
Copy link

Change https://go.dev/cl/408874 mentions this issue: doc: add release note for reflect.Value.{Bytes,Len,Cap}

gopherbot pushed a commit that referenced this issue May 30, 2022
Update #47066
Update #52411

Change-Id: I85139d774c16c9e6d1a2592a5abba58a49338674
Reviewed-on: https://go-review.googlesource.com/c/go/+/408874
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
@golang golang locked and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

8 participants