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

sync: add Map.Clear method #61696

Closed
leaxoy opened this issue Aug 1, 2023 · 13 comments
Closed

sync: add Map.Clear method #61696

leaxoy opened this issue Aug 1, 2023 · 13 comments

Comments

@leaxoy
Copy link

leaxoy commented Aug 1, 2023

Since we have landed #56351 for clear builtin map, but we seems forget the sync variant sync.Map, let's do it!

@leaxoy leaxoy added the Proposal label Aug 1, 2023
@gopherbot gopherbot added this to the Proposal milestone Aug 1, 2023
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Aug 1, 2023
@ianlancetaylor
Copy link
Contributor

CC @bcmills

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/515015 mentions this issue: sync/map: clear

@leaxoy
Copy link
Author

leaxoy commented Aug 4, 2023

@ianlancetaylor Maybe this should a release blocker for 1.21?

@ianlancetaylor
Copy link
Contributor

Unfortunately it's too late for 1.21. And I don't see why it would be a release blocker anyhow. While I agree that the predeclared clear function is a good argument for adding a sync.Map.Clear method, they are two different things.

@leaxoy
Copy link
Author

leaxoy commented Aug 5, 2023

All right, my idea is that since the built-in clear operation is provided, sync version should also provide the same capability, but you said that it is too late for 1.21, it is make sense, after all, we can work without clear for many years.

Anyway, it's useful to land on 1.22 and it's not too late.

@bcmills
Copy link
Contributor

bcmills commented Aug 8, 2023

I agree that a Clear method seems reasonable, given the built-in function.

That said, since sync.Map is sensitive to the set of keys that have recently appeared in the map, should Map.Clear retain (empty) entries for those keys so that they will be efficient to store again? Or should it be equivalent to calling Delete sequentially, which only leaves some fraction of the existing keys in the map? Or should it reset all of the keys completely?

This interacts in interesting ways with existing Map performance issues, particularly:

@rsc
Copy link
Contributor

rsc commented Aug 9, 2023

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 Aug 9, 2023
@rsc
Copy link
Contributor

rsc commented Aug 16, 2023

That said, since sync.Map is sensitive to the set of keys that have recently appeared in the map, should Map.Clear retain (empty) entries for those keys so that they will be efficient to store again? Or should it be equivalent to calling Delete sequentially, which only leaves some fraction of the existing keys in the map? Or should it reset all of the keys completely?

I think "delete everything" is fine.

@rsc
Copy link
Contributor

rsc commented Aug 16, 2023

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 Aug 16, 2023
@rsc
Copy link
Contributor

rsc commented Aug 30, 2023

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

@rsc rsc moved this from Likely Accept to Accepted in Proposals Aug 30, 2023
@rsc rsc changed the title proposal: sync: add Map.Clear method sync: add Map.Clear method Aug 30, 2023
@rsc rsc modified the milestones: Proposal, Backlog Aug 30, 2023
@qiulaidongfeng
Copy link
Member

The time to freeze the code is very close. Should CL515051 be merged into go1.22?

@bcmills bcmills modified the milestones: Backlog, Go1.23 Dec 12, 2023
@DenizChildir
Copy link

dam, I just saw this in the golang std reference and thought it was an actual function.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/561755 mentions this issue: doc/next: add release note for sync/Map.Clear

gopherbot pushed a commit that referenced this issue Feb 7, 2024
Fix https://go-review.git.corp.google.com/c/go/+/561456

For #61696

Change-Id: I573bd14cf0e5c41adcc8c2b9481e85c5792c2a82
GitHub-Last-Rev: c6fce6d
GitHub-Pull-Request: #65534
Reviewed-on: https://go-review.googlesource.com/c/go/+/561755
Reviewed-by: David Chase <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Fixes golang#61696

Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 17bedc8
GitHub-Pull-Request: golang#61702
Reviewed-on: https://go-review.googlesource.com/c/go/+/515015
Auto-Submit: Bryan Mills <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
Reviewed-by: qiulaidongfeng <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Fix https://go-review.git.corp.google.com/c/go/+/561456

For golang#61696

Change-Id: I573bd14cf0e5c41adcc8c2b9481e85c5792c2a82
GitHub-Last-Rev: c6fce6d
GitHub-Pull-Request: golang#65534
Reviewed-on: https://go-review.googlesource.com/c/go/+/561755
Reviewed-by: David Chase <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

7 participants