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

Open
dsnet opened this issue Apr 15, 2022 · 18 comments
Open

reflect: add Value.SetZero #52376

dsnet opened this issue Apr 15, 2022 · 18 comments

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Apr 15, 2022

This is a re-proposal of #33136, which was converted into a performance optimization where reflect.Zero does not allocate. That change certainly helps, but is insufficient especially when trying to zero out small values.

I propose we add a dedicated Value.SetZero method.

A prototype implementation of mine shows that this is much faster for small values:

Bool:       3.9x faster
Int:        3.5x faster
Uint:       3.9x faster
Float:      3.6x faster
Pointer:    2.2x faster
Map:        2.3x faster
Slice:      3.1x faster
Interface:  1.7x faster
String:     3.3x faster

Unfortunately, there is no convenient SetXXX API for clearing nil-able types (e.g., pointers, slices, and maps) and I was going to propose a Value.SetNil method, but we might as well expand that handle clearing all kinds.

@ianlancetaylor ianlancetaylor changed the title reflect: add Value.SetZero proposal: reflect: add Value.SetZero Apr 17, 2022
@gopherbot gopherbot added this to the Proposal milestone Apr 17, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 17, 2022

CC @randall77

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Apr 17, 2022
@randall77
Copy link
Contributor

@randall77 randall77 commented Apr 18, 2022

Seems fine to me.

@AAlvarez90
Copy link

@AAlvarez90 AAlvarez90 commented Apr 24, 2022

@dsnet Are you going to try to tackle this for 1.19?

@zaunist
Copy link

@zaunist zaunist commented Apr 24, 2022

Value.SetZeroValue.SetNil method and the corresponding test case need to be added, right? If that's all, I'm happy to do it :)

@AAlvarez90
Copy link

@AAlvarez90 AAlvarez90 commented Apr 24, 2022

@zaunist It is a start but I think @dsnet may have something else planned. Anyways. Any safe performance improvement is always welcome.

@dsnet
Copy link
Member Author

@dsnet dsnet commented Apr 24, 2022

@dsnet Are you going to try to tackle this for 1.19?

I have an implementation ready, but this needs to go through the proposal process. There is no guarantee that this gets accepted.

@AAlvarez90

This comment was marked as off-topic.

@randall77

This comment was marked as off-topic.

@ianlancetaylor

This comment was marked as off-topic.

@AAlvarez90

This comment was marked as off-topic.

@mvdan

This comment was marked as off-topic.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 1, 2022

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 Jun 1, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Jun 8, 2022

@dsnet, can you confirm that you are comparing with the v.Set(reflect.Zero(v.Type())) that has been optimized as in #33136 and that that expression is not allocating?

Assuming yes, then adding SetZero for the final 2-4X seems OK.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 10, 2022

Change https://go.dev/cl/411476 mentions this issue: reflect: add Value.SetZero

@dsnet
Copy link
Member Author

@dsnet dsnet commented Jun 10, 2022

Given that #33136 has been around since Go1.16, I'm fairly sure I'm accounting from the benefits of that optimization.

In https://go.dev/cl/411476, the performance numbers are as follows:

                     Direct         CachedZero     NewZero
SetZero/Bool         2.20ns ± 0%    8.97ns ± 5%    11.4ns ± 1%
SetZero/Int          3.08ns ± 1%    9.06ns ± 1%    11.5ns ± 0%
SetZero/Uint         2.88ns ± 1%    9.04ns ± 1%    11.7ns ± 5%
SetZero/Float        2.65ns ± 2%    9.05ns ± 1%    11.5ns ± 1%
SetZero/Complex      2.68ns ± 3%    9.31ns ± 1%    11.7ns ± 1%
SetZero/Array        6.69ns ± 4%    9.32ns ± 1%    11.8ns ± 1%
SetZero/Chan         3.31ns ± 1%    6.19ns ± 1%    8.20ns ± 1%
SetZero/Func         3.32ns ± 1%    6.20ns ± 0%    8.24ns ± 1%
SetZero/Any          2.64ns ± 1%    9.39ns ± 3%    11.8ns ± 2%
SetZero/Interface    2.66ns ± 1%    9.31ns ± 1%    11.8ns ± 1%
SetZero/Map          3.31ns ± 1%    6.21ns ± 2%    8.19ns ± 1%
SetZero/Pointer      3.30ns ± 1%    6.22ns ± 1%    8.17ns ± 1%
SetZero/Slice        2.90ns ± 4%    9.13ns ± 1%    11.6ns ± 1%
SetZero/String       3.11ns ± 1%    9.30ns ± 1%    11.8ns ± 2%
SetZero/Struct       6.37ns ± 1%    9.18ns ± 4%    11.5ns ± 1%

where:

  • Direct is the performance of v.SetZero()
  • CachedZero is the performance of v.Set(zero) where zero is a pre-computed variable with Zero(v.Type())
  • NewZero is the performance of v.Set(Zero(v.Type()))

we can see that Direct is generally 2-4x faster.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 15, 2022

Thanks for the numbers. This seems fine now that we understand the justification well.

@rsc rsc moved this from Active to Likely Accept in Proposals Jun 15, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Jun 15, 2022

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 Jun 22, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Jun 22, 2022

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: add Value.SetZero reflect: add Value.SetZero Jun 22, 2022
@rsc rsc removed this from the Proposal milestone Jun 22, 2022
@rsc rsc added this to the Backlog milestone Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Development

No branches or pull requests

8 participants