reflect: rename MapIter.SetKey/SetValue -> Value.SetIterKey/SetIterValue

[dsnet]

(copying my comment from #46131 (comment) to avoid it being forgotten for the 1.18 release)

Bikeshed: I was a little confused by the direction of setting. In reflect, we have:

func (v Value) Set(x Value)
func (v Value) SetBool(x bool)
func (v Value) SetBytes(x []byte)
func (v Value) SetCap(n int)
func (v Value) SetComplex(x complex128)
func (v Value) SetFloat(x float64)
func (v Value) SetInt(x int64)
func (v Value) SetLen(n int)
func (v Value) SetMapIndex(key, elem Value)
func (v Value) SetPointer(x unsafe.Pointer)
func (v Value) SetString(x string)
func (v Value) SetUint(x uint64)

All of these store a value into the receiver v from the input argument.

Now we added:

func (it *MapIter) SetKey(dst Value)
func (it *MapIter) SetValue(dst Value)

However, contrary to reflect.Value.SetXXX, a value is being stored into the input argument from state in the receiver it. It is the opposite direction.

Perhaps we should rename it as SetKeyInto and SetValueInto? Or StoreKey and StoreValue (per @josharian's suggestion)?

\cc @rsc @randall77

[randall77]

Or even,

func (v Value) SetKey(it *MapIter)
func (v Value) SetValue(it *MapIter)

?

[dsnet]

I like that even better :)

[dsnet]

Although, you probably want to add Map in the name:

func (v Value) SetMapKey(it *MapIter)
func (v Value) SetMapValue(it *MapIter)

since the other map functionality have Map in the name: Value.MapIndex, Value.MapKeys, Value.MapRange, Value.SetMapIndex.

[randall77]

Or SetIterKey/SetIterValue.

[dsnet]

I was looking at sources of allocations using reflect. Another that I found was something of the following pattern:

v.Set(m.MapIndex(k))

Ideally, we would be able to do this with an allocation-free API. Perhaps:

v.SetMapIndex(m, k)

Whatever names we chose here, I would hope that it doesn't steal a good name from this possible API.

[rsc]

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]

The proposal seems to be to rename the functions so that

v.SetIterKey(it) is the same as (but more efficient than) v.Set(it.Key())
v.SetIterValue(it) is the same as (but more efficient than) v.Set(it.Value())

Is that right? Is there some kind of trick we can do in v.Set to enable these kinds of optimizations invisibly?

[dsnet]

Is there some kind of trick we can do in v.Set to enable these kinds of optimizations invisibly?

That would be ideal. #48000 is the same class of problem with a similar proposed solution.

[randall77]

I'm not sure we can, as it.Key() and it.Value() are what are doing the allocations. Only when they are passed directly to Set() and nowhere else is the optimization possible.

I guess the compiler could have a special case for reflect.Set(it.Key()) (and perhaps other Set variants?) that calls into an unexported setIterKey in reflect. We haven't gone down the road of semantic-inlining the reflect package yet.

[dsnet]

If the part of it.Key that allocates is inlinable, then maybe it's possible that calling v.Set(it.Key()) doesn't have to allocate on the heap because escape analysis can prove that the input to v.Set doesn't escape.

[randall77]

it.Key() needs to allocate on the heap regardless of escape analysis because the size it needs to allocate is not constant.

[randall77]

In order to avoid a copy, we need to know that no map write operation happens between it.Key() and the Set(). The only way I can see knowing that is to detect Set(it.Key()) in the compiler and rewrite that. Or make the user tell us using SetIterKey.

[rsc]

Thanks for the discussion about optimizations. It does seem like SetIterKey and SetIterValue are the way to go.

[rsc]

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

[rsc]

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

[gopherbot]

Change https://golang.org/cl/356049 mentions this issue: reflect: rename Mapiter.SetKey to Value.SetIterKey

[gopherbot]

Change https://golang.org/cl/356050 mentions this issue: doc: document new reflect.SetIter{Key,Value} functions