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: add Value.SetZero #33136

Open
dsnet opened this issue Jul 16, 2019 · 10 comments

Comments

@dsnet
Copy link
Member

commented Jul 16, 2019

The Value API lacks an efficient way to zero out a value. Currently, one can do v.Set(reflect.Zero(v.Type())). However, this is not efficient since reflect.Zero may need to allocate a large object, and also reflect.Value.Set will use runtime.typedmemmove under the hood instead of the more efficient runtime.typedmemclr.

I propose adding reflect.Value.SetZero method as a more efficient way to set the value to the zero value.

@gopherbot gopherbot added this to the Proposal milestone Jul 16, 2019

@gopherbot gopherbot added the Proposal label Jul 16, 2019

@martisch

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

I would also like to see this added and want to give some context where this can have a big effect with a use case:
I came across this "inefficiency" also recently when looking into code for a proto sanitizer for log writing that should wipe out sensitive fields and therefore needs to "unset"/"wipe" some proto fields to a zero value. This was using reflecting on structs. Creating zero values was showing up in profiling for a non trivial amount of time and memory allocation. A future new proto API might provide some better method but then that might also be able or need to have an efficient reflect method under the hood.

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

A future new proto API might provide some better method but then that might also be able or need to have an efficient reflect method under the hood.

Heh. Ironically, I filed this issue in trying to optimize the internal protobuf implementation itself and couldn't figure out an efficient to this.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Given v.Set(reflect.Zero(v.Type())), the place where the inefficiency happens is if v.Type() is a value larger than a single word, so that the reflect.Value contains a pointer to a zeroed allocation.

I think we could change the representation of reflect.Value to define that if that pointer is nil, it is interpreted as pointing to an appropriate number of zeroed bytes. Then the idiom in question starts being more efficient without any changes to client code, no new API to learn, and so on.

The change seems like it could be done completely invisibly. The Value containing a nil pointer could not be addressable, but the result of reflect.Zero is not addressable anyway, so that wouldn't be a problem.

Should we think about doing that invisible optimization instead of new API?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

I tried a simple version of that optimization and it did save 75% of the time on this benchmark. So the idea does seem worth pursuing. @dsnet Is this a plausible benchmark for the kinds of code you are concerned about?

(It's not a tiny change to the reflect package, diffstat reports 159 insertions, 35 deletions.)

func BenchmarkZero(b *testing.B) {
	t := TypeOf(struct {a, b, c, d *byte}{})
	v := New(t)
	for i := 0; i < b.N; i++ {
		v.Elem().Set(Zero(t))
	}
}
@dsnet

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

Do you have a CL for this experiment that I can take for a spin?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Aug 21, 2019

Change https://golang.org/cl/191199 mentions this issue: reflect: treat nil Value ptr as zero value of type

@randall77

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

The runtime already has a 1K region of zeros that we can use to be the backing store for the result of reflect.Zero.
The change would then be as simple as:

	if ifaceIndir(t) {
		var p unsafe.Pointer
		if t.size <= maxZero {
			p = unsafe.Pointer(&zeroVal[0])
		} else {
			p = unsafe_New(t)
		}
		return Value{t, p, fl | flagIndir}
	}

// Buffer of zeros. We could share this with runtime.zeroVal with linkname tricks, or keep it separate.
const maxZero = 1024
var zeroVal [maxZero]byte

We could also detect this particular buffer in Set and use typedmemclr instead of typedmemmove.

It would only require allocation in the >1K case, which I think is rare.

@mvdan

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

This is also a big inefficiency in packages like encoding/json. Please let me know when a CL is ready and I'll be happy to help review and test.

If this can be done without new API, all the better :)

@gopherbot

This comment has been minimized.

Copy link

commented Aug 29, 2019

Change https://golang.org/cl/192331 mentions this issue: reflect: use zero buffer to back the Value returned by Zero

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.