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: rename MapIter.SetKey/SetValue -> Value.SetIterKey/SetIterValue #48294

Closed
dsnet opened this issue Sep 9, 2021 · 17 comments
Closed

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

dsnet opened this issue Sep 9, 2021 · 17 comments

Comments

@dsnet
Copy link
Member

dsnet commented Sep 9, 2021

(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

@dsnet dsnet added this to the Go1.18 milestone Sep 9, 2021
@randall77
Copy link
Contributor

Or even,

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

?

@dsnet
Copy link
Member Author

dsnet commented Sep 9, 2021

I like that even better :)

@dsnet
Copy link
Member Author

dsnet commented Sep 9, 2021

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.

@ianlancetaylor ianlancetaylor changed the title reflect: rename MapIter.SetKey and MapIter.SetValue proposal: reflect: rename MapIter.SetKey and MapIter.SetValue Sep 9, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Sep 9, 2021
@randall77
Copy link
Contributor

Or SetIterKey/SetIterValue.

@dsnet
Copy link
Member Author

dsnet commented Sep 11, 2021

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 rsc moved this from Incoming to Active in Proposals (old) Sep 15, 2021
@rsc
Copy link
Contributor

rsc commented Sep 15, 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
Copy link
Contributor

rsc commented Sep 22, 2021

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
Copy link
Member Author

dsnet commented Sep 22, 2021

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
Copy link
Contributor

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
Copy link
Member Author

dsnet commented Sep 22, 2021

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
Copy link
Contributor

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

@randall77
Copy link
Contributor

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
Copy link
Contributor

rsc commented Oct 6, 2021

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

@rsc rsc changed the title proposal: reflect: rename MapIter.SetKey and MapIter.SetValue proposal: reflect: rename MapIter.SetKey -> Value.SetIterKey, and also s/Key/Value/ Oct 6, 2021
@rsc
Copy link
Contributor

rsc commented Oct 6, 2021

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

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Oct 6, 2021
@rsc rsc changed the title proposal: reflect: rename MapIter.SetKey -> Value.SetIterKey, and also s/Key/Value/ proposal: reflect: rename MapIter.SetKey/SetValue -> Value.SetIterKey/SetIterValue Oct 13, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Oct 13, 2021
@rsc
Copy link
Contributor

rsc commented Oct 13, 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: rename MapIter.SetKey/SetValue -> Value.SetIterKey/SetIterValue reflect: rename MapIter.SetKey/SetValue -> Value.SetIterKey/SetIterValue Oct 13, 2021
@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Oct 15, 2021
Update #48294
Update #47694

Change-Id: I4d4c01be74a9736d89a4ec92318ce29ff7289a0d
Reviewed-on: https://go-review.googlesource.com/c/go/+/356050
Trust: Keith Randall <khr@golang.org>
Reviewed-by: Joe Tsai <joetsai@digital-static.net>
@golang golang locked and limited conversation to collaborators Oct 15, 2022
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

4 participants