[go] sync/map: clear
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
sync/map: clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 5b099eea95fa759a9d3468d4e7bec19aba0bc6ce
GitHub-Pull-Request: golang/go#61702
---
M src/sync/map.go
M src/sync/map_test.go
2 files changed, 83 insertions(+), 0 deletions(-)
diff --git a/src/sync/map.go b/src/sync/map.go
index e8ccf58..eb93a1f 100644
--- a/src/sync/map.go
+++ b/src/sync/map.go
@@ -155,6 +155,18 @@
_, _ = m.Swap(key, value)
}
+func (m *Map) Clear() {
+ defer m.mu.Unlock()
+ m.mu.Lock()
+
+ read := m.loadReadOnly()
+
+ clear(read.m) // FIXME: e.delete()
+
+ clear(m.dirty)
+
+}
+
// tryCompareAndSwap compare the entry with the given old value and swaps
// it with a new value if the entry is equal to the old value, and the entry
// has not been expunged.
diff --git a/src/sync/map_test.go b/src/sync/map_test.go
index 1eb3fc6..cde91e3 100644
--- a/src/sync/map_test.go
+++ b/src/sync/map_test.go
@@ -280,3 +280,74 @@
t.Fatalf("CompareAndSwap on an non-existing key succeeded")
}
}
+
+func TestMapClear(t *testing.T) {
+
+ var myMap sync.Map
+
+ key := "go"
+ val := 1.21
+ myMap.Store(key, val)
+ loadedVal, ok := myMap.Load(key)
+
+ if !ok {
+ t.Fatalf("Store failed to store- %v:%v", key, val)
+ }
+
+ if loadedVal != val {
+ t.Fatalf("Load: invalid value- %v:%v", key, loadedVal)
+ }
+
+ myMap.Clear()
+
+ nilVal, ok := myMap.Load(key)
+
+ if nilVal != nil {
+ t.Fatalf("Clear: failed %v:%v", key, nilVal)
+ }
+
+ if !ok {
+ t.Fatalf("Clear: failed %v:%v", key, nilVal)
+ }
+}
+
+func TestMapClearRace(t *testing.T) {
+ var myMap sync.Map
+
+ wg := sync.WaitGroup{}
+ wg.Add(30) // 10 goroutines for writing, 10 goroutines for reading, 10 goroutines for waiting
+
+ // Writing data to the map concurrently
+ for i := 0; i < 10; i++ {
+ go func(key, value int) {
+ defer wg.Done()
+ myMap.Store(key, value)
+ }(i, i*10)
+ }
+
+ // Reading data from the map concurrently
+ for i := 0; i < 10; i++ {
+ go func(key int) {
+ defer wg.Done()
+ if value, ok := myMap.Load(key); ok {
+ t.Logf("Key: %v, Value: %v\n", key, value)
+ } else {
+ t.Logf("Key: %v not found\n", key)
+ }
+ }(i)
+ }
+
+ // Clearing data from the map concurrently
+ for i := 0; i < 10; i++ {
+ go func() {
+ defer wg.Done()
+ // myMap.Clear()
+ }()
+ }
+ myMap.Clear()
+
+ wg.Wait()
+
+ myMap.Clear()
+
+}
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
Gerrit Bot uploaded patch set #2 to this change.
sync/map: clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: afc46339743decf58d5ac51d868eb0ee26884a75
GitHub-Pull-Request: golang/go#61702
---
M src/sync/map.go
M src/sync/map_test.go
2 files changed, 117 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
Gerrit Bot uploaded patch set #3 to this change.
sync/map: clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: f268be33d5fe309a435c338a84f4eb65e2d9a633
GitHub-Pull-Request: golang/go#61702
---
M src/sync/map.go
M src/sync/map_test.go
2 files changed, 123 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
1 comment:
Patchset:
I came across a failing test. Want to realize if its a bug
func TestMapDelete(t *testing.T) {
var myMap sync.Map
key := "go"
val := 1.21
myMap.Store(key, val)
loadedVal, ok := myMap.Load(key)if !ok {
t.Fatalf("Store failed to store- %v:%v", key, val)
}
if loadedVal != val {
t.Fatalf("Load: invalid value- %v:%v", key, loadedVal)
}
myMap.Delete(key)
nilVal, ok := myMap.Load(key)
if nilVal != nil {
t.Fatalf("Delete: failed %v:%v", key, nilVal)
}if !ok { // FIXME: failing test
t.Fatalf("Delete: failed found val %v:%v", key, nilVal)
}
}
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
Gerrit Bot uploaded patch set #4 to this change.
sync/map: clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: e68759e896c56a155b5cde08fe591d487a5f460d
GitHub-Pull-Request: golang/go#61702
---
M src/sync/map.go
M src/sync/map_test.go
2 files changed, 127 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
Gerrit Bot uploaded patch set #5 to this change.
sync/map: clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 632ea9f865daa74446b13004ce4133edbdeee6f5
GitHub-Pull-Request: golang/go#61702
---
M src/sync/map.go
M src/sync/map_test.go
2 files changed, 119 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
1 comment:
File src/sync/map.go:
Patch Set #3, Line 159:
defer m.mu.Unlock()The right implementation is probably: […]
Acknowledged
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
1 comment:
Patchset:
myMap.Delete(key)
nilVal, ok := myMap.Load(key)
if !ok { // FIXME: failing test
t.Fatalf("Delete: failed found val %v:%v", key, nilVal)
}
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
Gerrit Bot uploaded patch set #6 to this change.
sync/map: clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: dcb07ff29030f7b69f0a8e5a51ba22b62f60f5cc
GitHub-Pull-Request: golang/go#61702
---
M src/sync/map.go
M src/sync/map_test.go
2 files changed, 125 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
1 comment:
Patchset:
myMap.Delete(key) […]
Never mind I made a typo !ok should have ok
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
1 comment:
Patchset:
Never mind I made a typo !ok should have ok
my bad!
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
Gerrit Bot uploaded patch set #7 to this change.
sync/map: Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 277176f6dbe77186f4aacba70bb99b9f25747658
GitHub-Pull-Request: golang/go#61702
---
M src/sync/map.go
M src/sync/map_test.go
2 files changed, 122 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
qiulaidongfeng (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
4 comments:
Patchset:
There is a merge conflict, please run git codereview sync.
Commit Message:
See https://go.dev/doc/contribute#ref_issues this CL has completed the proposal API and testing, please use Fixes.
File src/sync/map_test.go:
Patch Set #7, Line 333:
myMap.Delete(key)Why call Delete when Clear has already been called?
myMap.Store(key, val)
if val1, ok := myMap.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}Why re store the cleared data?
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
qiulaidongfeng (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox.
1 comment:
Patchset:
There is a merge conflict, please run git codereview sync.
I pressed the wrong button, it didn't solve it.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox, qiulaidongfeng.
1 comment:
Patchset:
I pressed the wrong button, it didn't solve it.
ok
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox, qiulaidongfeng.
1 comment:
Patchset:
done resolved. Will take some time for gopher bot to sync
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox, qiulaidongfeng.
2 comments:
File src/sync/map_test.go:
Patch Set #7, Line 333:
myMap.Delete(key)Why call Delete when Clear has already been called?
yeah i think delete is unnecessary
myMap.Store(key, val)
if val1, ok := myMap.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}Why re store the cleared data?
Store: failed after clear. Test
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #8 to this change.
sync/map: Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 9c11c75aca0e5f8d035da159d75dc44ba4ae970c
GitHub-Pull-Request: golang/go#61702
---
M src/sync/map.go
M src/sync/map_test.go
2 files changed, 120 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Russ Cox, qiulaidongfeng.
5 comments:
Patchset:
You'll need to add an entry in $GOROOT/api/next for the new method. Follow the existing format in that file. Test it by running "go tool dist test -run=api".
Commit Message:
Patch Set #8, Line 7:
sync/map: Clearsync: add Map.Clear
File src/sync/map.go:
Patch Set #8, Line 158:
func (m *Map) Clear() {This method needs a doc comment.
Remove this blank line and the one just before the right curly brace.
File src/sync/map_test.go:
Remove this blank line.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
4 comments:
Commit Message:
Patch Set #8, Line 7:
sync/map: Clear
sync: add Map. […]
done
File src/sync/map.go:
Patch Set #8, Line 158:
func (m *Map) Clear() {This method needs a doc comment.
done
Remove this blank line and the one just before the right curly brace.
done
File src/sync/map_test.go:
Remove this blank line.
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #9 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 643c116b2e8b3a6ae8addee674ffbcbb983f44fe
GitHub-Pull-Request: golang/go#61702
---
M src/sync/map.go
M src/sync/map_test.go
2 files changed, 119 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Russ Cox, qiulaidongfeng.
1 comment:
Patchset:
You'll need to add an entry in $GOROOT/api/next for the new method. […]
I am getting:
```
go tool dist test -run=api
warning: GOPATH set to GOROOT (/Users/hiro/goferHiro/go) has no effect
warning: GOPATH set to GOROOT (/Users/hiro/goferHiro/go) has no effect
warning: GOPATH set to GOROOT (/Users/hiro/goferHiro/go) has no effect
warning: GOPATH set to GOROOT (/Users/hiro/goferHiro/go) has no effect##### Test execution environment.
warning: GOPATH set to GOROOT (/Users/hiro/goferHiro/go) has no effect
# GOARCH: arm64
# CPU:
# GOOS: darwin
# OS Version: Darwin 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:39:35 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T8103 arm64##### Testing packages.
warning: GOPATH set to GOROOT (/Users/hiro/goferHiro/go) has no effect
2023/09/08 01:39:22 go list: invalid output: invalid character 'w' looking for beginning of value
FAIL cmd/api 0.683s
FAIL
go tool dist: Failed: exit status 1```
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #10 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 474255c26c7bd9735dcb1214ea44975ee5be833f
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_test.go
3 files changed, 120 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
Patchset:
I am getting: […]
do i need to add in src/cmd/api/testdata?
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Russ Cox, qiulaidongfeng.
1 comment:
Patchset:
do i need to add in src/cmd/api/testdata?
You shouldn't need to change anything in cmd/api/testdata. I don't know what that "go list" error means. To avoid the GOPATH errors set the environment variable GOPATH to some other directory. The default for GOPATH is $HOME/go, but you are using that for GOROOT.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
Patchset:
You shouldn't need to change anything in cmd/api/testdata. […]
The error is from `cmd/api/main_test.go` trying to parse erroneously-combined output from `go list`:
https://cs.opensource.google/go/go/+/master:src/cmd/api/main_test.go;l=493;drc=5a3048bf0eefd2f99382a980f975d6a1fb6b921aThe fix for that failure mode is to use `cmd.Output()` instead of `cmd.CombinedOutput()` there:
```go
out, err := cmd.Output()
if err != nil {
if ee, ok := err.(*exec.ExitError); ok && len(ee.Stderr) > 0 {
log.Fatalf("loading imports: %v: %v\n%s", cmd, err, ee.Stderr)
}
log.Fatalf("loading imports: %v: %v", cmd, err)
}
```
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Russ Cox, qiulaidongfeng.
1 comment:
Patchset:
The error is from `cmd/api/main_test. […]
Thanks, CL 526775.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Russ Cox, qiulaidongfeng.
Patch set 10:Commit-Queue +1
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
Patchset:
Thanks, CL 526775.
Thanks Ian Lance Taylor: when I updated GOPATH. the problem was resolved
```
go tool dist test -run=api
##### Test execution environment.
# GOARCH: arm64
# CPU:
# GOOS: darwin
# OS Version: Darwin 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:39:35 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T8103 arm64
##### Testing packages.
ok cmd/api 0.696s##### API check
ok cmd/api 10.278sALL TESTS PASSED (some were excluded)
```
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #11 to this change.
The following approvals got outdated and were removed: LUCI-TryBot-Result-1 by Go LUCI
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 6e06a5475ca2e5a33aab8161e8b1cc116f5a1ebf
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_test.go
3 files changed, 120 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Russ Cox, qiulaidongfeng.
Patch set 11:Commit-Queue +1
qiulaidongfeng (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
1 comment:
File src/sync/map_test.go:
myMap.Store(key, val)
if val1, ok := myMap.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}
See https://pkg.go.dev/testing#T.Fatalf and https://pkg.go.dev/testing#T.FailNow , If Clear fails, it will not execute here. Please delete this paragraph.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
qiulaidongfeng (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox.
1 comment:
Patchset:
ok
Acknowledged
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
myMap.Store(key, val)
if val1, ok := myMap.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}
See https://pkg.go.dev/testing#T.Fatalf and https://pkg.go.dev/testing#T. […]
Store failing after clear succeeded
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
qiulaidongfeng (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox.
myMap.Store(key, val)
if val1, ok := myMap.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}
Store failing after clear succeeded
Do you mean that after clearing, testing can store the same data? If so, please add a comment to explain.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
myMap.Store(key, val)
if val1, ok := myMap.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}
Do you mean that after clearing, testing can store the same data? If so, please add a comment to exp […]
I thought the error message -"Store failed after clear" explains it. But yeah if its misleading perhaps we can have a comment
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
qiulaidongfeng (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox.
myMap.Store(key, val)
if val1, ok := myMap.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}
I thought the error message -"Store failed after clear" explains it. […]
English is not my native language, and TestMapClear looks more like testing whether Clear can clear data.
I think there are three solutions to the current situation:
1. Change the name of the test to indicate that it will also test whether the same data can be stored after clearing.
2. Add a comment to indicate that it will also test whether the same data can be stored after clearing.
3. If you think it's a misunderstanding caused by my limited English proficiency, you can press the Done button on the dialog box to resolve this conversation.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox.
Will wait for a second opinion then.
1 comment:
Patchset:
Will wait for a second opinion then.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
myMap.Store(key, val)
if val1, ok := myMap.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}
English is not my native language, and TestMapClear looks more like testing whether Clear can clear […]
I think adding a comment on L352 should clear the confusion. Just in case, will wait for a second opinion then.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
qiulaidongfeng (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox.
myMap.Store(key, val)
if val1, ok := myMap.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}
I think adding a comment on L352 should clear the confusion. […]
I think it's good.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox.
11 comments:
File src/sync/map.go:
(nit) Don't start a method body with a blank line.
defer m.mu.Unlock()
m.mu.Lock()Put the `m.mu.Lock` before the `defer m.mu.Unlock()`, so that the user gets a cleaner stack trace if `Lock` panics due to a nil receiver.
Patch Set #11, Line 164:
m.read.Store(new(readOnly))We should avoid allocating a new `readOnly` here if the map was already empty and not amended.
(And this should be confirmed with a test using `testing.AllocsPerRun`; see the existing `TestMapRangeNoAllocations`.)
Patch Set #11, Line 166:
clear(m.dirty)Also set `m.misses = 0`, so that we don't immediately promote the newly-cleared `dirty` map on the next operation.
Putting it all together:
```
read := m.loadReadOnly()
if len(read.m) == 0 && !read.amended {
return
}m.mu.Lock()
defer m.mu.Unlock()read = m.loadReadOnly()
if len(read.m) > 0 || read.amended {
m.read.Store(&readOnly{})
}
clear(m.dirty)
m.misses = 0
```
File src/sync/map_test.go:
Patch Set #11, Line 299:
var myMap sync.Map(https://go.dev/wiki/CodeReviewComments#variable-names)
We don't typically use prefixes like `my` in Go code. (Name the map `m` like in the existing test cases.)
Patch Set #11, Line 310:
if loadedVal != val {(https://go.dev/wiki/CodeReviewComments#variable-names)
`v`, not `loadedVal`.
Patch Set #11, Line 316:
nilVal, ok := myMap.Load(key)`v, ok = m.Load(k)`
Patch Set #11, Line 327:
func TestMapClear(t *testing.T) {Rather than just testing `Clear` on its own, please add `Clear` methods to the other implementations in `map_reference_test.go`, an entry in the `mapOps` map, and a case in `mapCall.apply` so that the new method will be covered by the existing `TestMapMatchesRWMutex` and `TestMapMatchesDeepCopy` tests.
Remove leading blank line.
func TestMapClearRace(t *testing.T) {
var myMap sync.Map
wg := sync.WaitGroup{}
wg.Add(30) // 10 goroutines for writing, 10 goroutines for reading, 10 goroutines for waiting
// Writing data to the map concurrently
for i := 0; i < 10; i++ {
go func(key, value int) {
defer wg.Done()
myMap.Store(key, value)
}(i, i*10)
}
// Reading data from the map concurrently
for i := 0; i < 10; i++ {
go func(key int) {
defer wg.Done()
if value, ok := myMap.Load(key); ok {
t.Logf("Key: %v, Value: %v\n", key, value)
} else {
t.Logf("Key: %v not found\n", key)
}
}(i)
}
// Clearing data from the map concurrently
for i := 0; i < 10; i++ {
go func() {
defer wg.Done()
myMap.Clear()
}()
}
wg.Wait()
myMap.Clear()
myMap.Range(func(key, val any) bool {
t.Errorf("invalid %v:%v", key, val)
return true
})
}The implementation of `Clear` seems simple enough not to need such an involved test, but it could be helpful to have a benchmark with a `perG` function that alternates among `Clear` and `Store` and `Load` with consistent keys; see the existing benchmarks in `map_bench_test.go`.
(Note that the go project's `-race` builders also run each benchmark 1x during testing.)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #12 to this change.
The following approvals got outdated and were removed: LUCI-TryBot-Result+1 by Go LUCI
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 16207c84535db09b6d55788653d0075e5e0b5b62
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_test.go
3 files changed, 119 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #13 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 67aa04ac732a2cf7e99bc0c392837845b557606e
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_test.go
3 files changed, 130 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
8 comments:
Patchset:
gopherbot: might take some time to sync up....
File src/sync/map.go:
(nit) Don't start a method body with a blank line.
Done
defer m.mu.Unlock()
m.mu.Lock()
Put the `m.mu.Lock` before the `defer m.mu. […]
Done
Putting it all together: […]
Thanks, its done
File src/sync/map_test.go:
Patch Set #11, Line 299:
var myMap sync.Map
Patch Set #11, Line 310:
if loadedVal != val {
Patch Set #11, Line 316:
nilVal, ok := myMap.Load(key)
`v, ok = m. […]
Done
Remove leading blank line.
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #14 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 817eccc6a0af158ff30e002144598c72367f7d78
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_test.go
3 files changed, 129 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
1 comment:
File src/sync/map_test.go:
Patch Set #11, Line 327:
func TestMapClear(t *testing.T) {Rather than just testing `Clear` on its own, please add `Clear` methods to the other implementations […]
couldn't find mapOps in _ref_test.go . SO adding it to` mapInterface` . Untested..
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
2 comments:
File src/sync/map.go:
Patch Set #11, Line 164:
m.read.Store(new(readOnly))We should avoid allocating a new `readOnly` here if the map was already empty and not amended. […]
Done
Patch Set #11, Line 166:
clear(m.dirty)Also set `m. […]
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #15 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 140314fc1e15ddfe9aebad0deea5bfcb850a7bac
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
4 files changed, 144 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #16 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: e5cd229ebae671efb0940d8613571eb2095d97d9
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
4 files changed, 144 insertions(+), 0 deletions(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
The implementation of `Clear` seems simple enough not to need such an involved test, but it could be […]
The test was written to ensure that clear doesn't break on concurrent reads and writes.
Since the existing tests with clear doesn't do concurrent reads,writes and loads.
But I forgot to check if the values of the keys are corrupted when clear is run concurrently
Do u think: I should implement a similar version with clear: https://github.com/golang/go/blob/817eccc6a0af158ff30e002144598c72367f7d78/src/sync/map_test.go#L188
or remove this implementation.
FYI: this test was written to be tested with -race flag.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
File src/sync/map_test.go:
The test was written to ensure that clear doesn't break on concurrent reads and writes. […]
working on the pefG function
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #17 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 4a35900c5bc6e52709fa95ba3315a819bd040d21
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 177 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
working on the pefG function
I wrote the perfG function:
```
perG: func(b *testing.B, pb *testing.PB, i int, m mapInterface) {
for ; pb.Next(); i++ {
k, v := i, i
m.Clear()
m.Store(k, v)
v1, ok := m.Load(k)if !ok {
b.Errorf("failed to load %v", k)
b.Skip()
}if v1.(int) != v {
b.Errorf("expected %v, got %v", k, v)
b.Skip()
}}
},
``````
goos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 10000 905754 ns/op
BenchmarkClear/*sync_test.RWMutexMap
map_bench_test.go:555: failed to load 10000
map_bench_test.go:556:
map_bench_test.go:555: failed to load 70028
map_bench_test.go:556:
map_bench_test.go:555: failed to load 40143
map_bench_test.go:556:
map_bench_test.go:555: failed to load 60112
map_bench_test.go:556:
map_bench_test.go:555: failed to load 30153
map_bench_test.go:556:
map_bench_test.go:555: failed to load 20032
map_bench_test.go:556:
map_bench_test.go:555: failed to load 50461
map_bench_test.go:556:
--- FAIL: BenchmarkClear/*sync_test.RWMutexMap-8
BenchmarkClear/*sync.Map
map_bench_test.go:555: failed to load 1
map_bench_test.go:556:
map_bench_test.go:555: failed to load 100
map_bench_test.go:556:
--- FAIL: BenchmarkClear/*sync.Map-8
--- FAIL: BenchmarkClear
FAIL
```
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #18 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 5057ae03e18fdd8097c846d29c22e6b7125fe724
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 178 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #19 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 611741c47b3491ffc60f3eef00dd4a5243c2cddd
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 177 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
9 comments:
File src/sync/map.go:
Patch Set #19, Line 162:
// avoid allocating a new readOnly here if the map was already empty and not amended(https://go.dev/wiki/CodeReviewComments#comment-sentences)
Go comments should usually be complete sentences: start with a capital letter, and end with punctuation.
Patch Set #19, Line 162:
// avoid allocating a new readOnly here if the map was already empty and not amended(nit) This comment could be more concise or at a higher level: the code already indicates that the map is “empty and not amended”.
Perhaps:
```go
// Avoid allocating a new readOnly when the map is already clear.
return
```Patch Set #19, Line 173:
m.read.Store(new(readOnly))This line is redundant with line 171.
File src/sync/map_bench_test.go:
setup: func(_ *testing.B, m mapInterface) {
for i := 0; i < mapSize; i++ {
m.Store(i, i)
}
},This benchmark never reaches a steady state of keys, so there is no need to do work to prime it in the setup function.
Patch Set #19, Line 554:
if !ok {This looks racy — isn't it possible for another goroutine to reach its `Clear` after this goroutine's `Store`?
Patch Set #19, Line 556:
b.Skip()It's not safe to call `b.Skip` in a `perG` function, since those run in separate goroutines.
(nit) Remove trailing blank line.
File src/sync/map_reference_test.go:
Patch Set #19, Line 285:
m.dirty()This implementation is not correct. The `DeepCopyMap` needs an `m.clean.Store` at the end of each method to update the state.
In this case, there is also no need to call `m.dirty()` to copy the existing contents. Instead, store a nil map:
```
func (m *DeepCopyMap) Clear() {
m.mu.Lock()
defer m.mu.Unlock()
m.clean.Store((map[any]any)(nil))
}
```
File src/sync/map_test.go:
Patch Set #19, Line 31:
var mapOps = [...]mapOp{The `mapOps` map is here.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #20 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: a6777d1f398385ac8c1551c6d6d87624e451110f
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 176 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #21 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: c08eb087da5ca7b4a3a560a1790a9c3c2c70ac5f
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 169 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
9 comments:
File src/sync/map.go:
Patch Set #19, Line 162:
// avoid allocating a new readOnly here if the map was already empty and not amended
(nit) This comment could be more concise or at a higher level: the code already indicates that the m […]
Done
Patch Set #19, Line 162:
// avoid allocating a new readOnly here if the map was already empty and not amended
(https://go.dev/wiki/CodeReviewComments#comment-sentences) […]
Acknowledged
Patch Set #19, Line 173:
m.read.Store(new(readOnly))This line is redundant with line 171.
My bad missed it....
File src/sync/map_bench_test.go:
setup: func(_ *testing.B, m mapInterface) {
for i := 0; i < mapSize; i++ {
m.Store(i, i)
}
},
This benchmark never reaches a steady state of keys, so there is no need to do work to prime it in t […]
yes there is no need of setup
(nit) Remove trailing blank line.
Acknowledged
File src/sync/map_reference_test.go:
Patch Set #19, Line 285:
m.dirty()
This implementation is not correct. The `DeepCopyMap` needs an `m.clean. […]
Done. What bout the L152, implementation of CLear in RWMutex map
File src/sync/map_test.go:
Patch Set #11, Line 327:
func TestMapClear(t *testing.T) {
couldn't find mapOps in _ref_test.go . SO adding it to` mapInterface` . Untested..
myMap.Store(key, val)
if val1, ok := myMap.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}
I think it's good.
Acknowledged
File src/sync/map_test.go:
Patch Set #19, Line 31:
var mapOps = [...]mapOp{The `mapOps` map is here.
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #22 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 689e8750eec64df41284ecab745c348342394c6d
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 174 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #23 to this change.
sync: add Map.Clear
For #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: ebc1375b7ead16bcd7cb0b685ed03b152f07ac50
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 178 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
setup: func(_ *testing.B, m mapInterface) {
for i := 0; i < mapSize; i++ {
m.Store(i, i)
}
},
yes there is no need of setup
Done
Patch Set #19, Line 556:
b.Skip()
It's not safe to call `b.Skip` in a `perG` function, since those run in separate goroutines.
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #24 to this change.
sync: add Map.Clear
Fixes #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: d11b339b872dd47c87f186f49ab152da9fb33c5e
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 182 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
3 comments:
Commit Message:
See https://go. […]
Done
File src/sync/map_bench_test.go:
Patch Set #19, Line 554:
if !ok {This looks racy — isn't it possible for another goroutine to reach its `Clear` after this goroutine' […]
Done
File src/sync/map_test.go:
I wrote the perfG function: […]
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
7 comments:
File src/sync/map.go:
Patch Set #11, Line 164:
m.read.Store(new(readOnly))Done
I'm not seeing an `AllocsPerRun` test for this — has it been added?
File src/sync/map_bench_test.go:
Patch Set #24, Line 547:
b.StartTimer() // racy but to benchmark this is crucialThis mutex and timer manipulation is not appropriate in a benchmark — if a benchmark is not as simple as possible, it is likely to be too noisy or biased to be useful.
In general it is fine for a benchmark of a data structure (like `sync.Map`) to measure a mix of function calls, rather than just a single function call — real programs will execute a mix of function calls too, so that kind of benchmark will actually be more useful for measuring real-world performance.
That said: to avoid spurious allocations from the `Store` call, I would suggest using `i%256` instead of `i` so that the interface conversions for `Store` don't require extra allocations (see `runtime.staticuint64s`).
v1, ok := m.Load(k)
if !ok {
b.Logf("failed to load %v", k)
// b.Skip() Unsafe to call in perfG
continue
}
if v1.(int) != v {
b.Errorf("expected %v, got %v", k, v)
// b.Skip()
continue
}
}I suggest skipping the `Load` calls entirely in this benchmark — given concurrency there is no guarantee as to what `Load` will return, and we can meaningfully benchmark `Clear` without it.
File src/sync/map_reference_test.go:
Patch Set #19, Line 285:
m.dirty()
Done. […]
That one is correct: the `RWMutexMap` implementation is supposed to measure the difference between `map[any][any]` and `sync.Map`.
File src/sync/map_test.go:
func TestMapDelete(t *testing.T) {
var m sync.Map
k := "go"
val := 1.22
m.Store(k, val)
v, ok := m.Load(k)
if !ok {
t.Fatalf("Store failed to store- %v:%v", k, val)
}
if v != val {
t.Fatalf("Load: invalid value- %v:%v", k, v)
}
m.Delete(k)
v, ok = m.Load(k)
if v != nil {
t.Fatalf("Delete: failed %v:%v", k, v)
}
if ok {
t.Fatalf("Delete: failed found %v:%v", k, v)
}
}
func TestMapClear(t *testing.T) {
var m sync.Map
key := "go"
val := 1.21
m.Store(key, val)
v, ok := m.Load(key)
if !ok {
t.Fatalf("Store failed to store- %v:%v", key, val)
}
if v != val {
t.Fatalf("Load: invalid value- %v:%v", key, v)
}
m.Clear()
nilVal, ok := m.Load(key)
if nilVal != nil || ok {
t.Fatalf("Clear: failed %v:%v", key, nilVal)
}
m.Store(key, val)
if val1, ok := m.Load(key); !ok || val1 != val {
t.Fatalf("Store: failed after clear %v:%v", key, val1)
}
}
Given that `Delete` and `Clear` should be covered by `TestMapMatchesRWMutex` and `TestMapMatchesDeepCopy`, I don't think we need separate sequential tests for them. (Those tests should already exercise these sequences and more.)
Patch Set #24, Line 364:
func TestConcurrentClear(t *testing.T) {A comment for this test would be helpful — what properties is it checking for? What kind of failure modes would we expect to see in case of a bug?
Patch Set #24, Line 403:
invalid(https://go.dev/wiki/CodeReviewComments#useful-test-failures)
Instead of just “invalid”, describe what was expected or not, and the relevant calls that led up to that expectation. For example:
```go
t.Errorf("after Clear, Range yielded (%v, %v); want no calls", k, v)
```
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #25 to this change.
sync: add Map.Clear
Fixes #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 6bf739cb31fbbf22ba094e236faf118505d06f2b
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 178 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #26 to this change.
sync: add Map.Clear
Fixes #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 166f86fdcdd84ed7e745cbd4cc857a63108083fa
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 180 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
4 comments:
File src/sync/map.go:
Patch Set #11, Line 164:
m.read.Store(new(readOnly))
I'm not seeing an `AllocsPerRun` test for this — has it been added?
No, working on it
File src/sync/map_bench_test.go:
Patch Set #24, Line 547:
b.StartTimer() // racy but to benchmark this is crucial>This mutex and timer manipulation is not appropriate in a benchmark
Ok
i%256 fits 1 byte. So less no of allocations at `Store` and `Type Assertion`
ok
v1, ok := m.Load(k)
if !ok {
b.Logf("failed to load %v", k)
// b.Skip() Unsafe to call in perfG
continue
}
if v1.(int) != v {
b.Errorf("expected %v, got %v", k, v)
// b.Skip()
continue
}
}
I suggest skipping the `Load` calls entirely in this benchmark — given concurrency there is no guara […]
ok, comment or remove the code. Can I use `LoadOrStore` instead?
File src/sync/map_test.go:
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #27 to this change.
sync: add Map.Clear
Fixes #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 3b231bdc8e4303893d93b604206439835341f561
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 197 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Patch Set #11, Line 164:
m.read.Store(new(readOnly))
No, working on it
Wrote: AllocsPerRun for Clear based on Range: Got it passing
=== RUN TestMapClearNoAllocations
--- PASS: TestMapClearNoAllocations (0.00s)
PASS
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
File src/sync/map_test.go:
Patch Set #24, Line 364:
func TestConcurrentClear(t *testing.T) {A comment for this test would be helpful — what properties is it checking for? What kind of failure […]
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
File src/sync/map.go:
Patch Set #11, Line 164:
m.read.Store(new(readOnly))
Wrote: AllocsPerRun for Clear based on Range: Got it passing […]
Sorry i have made the change, but even why i took it off the tests are passing
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
File src/sync/map.go:
Patch Set #11, Line 164:
m.read.Store(new(readOnly))
Sorry i have made the change, but even why i took it off the tests are passing
I mean even why i commented the check for empty, tests were passing
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
File src/sync/map_test.go:
func TestMapDelete(t *testing.T) {
var m sync.Map
k := "go"
val := 1.22
m.Store(k, val)
v, ok := m.Load(k)
if !ok {
Given that `Delete` and `Clear` should be covered by `TestMapMatchesRWMutex` and `TestMapMatchesDeep […]
So integration test for Delete And Clear is not required, ok very well I don't see for load, store either. My bad!!
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
File src/sync/map_test.go:
So integration test for Delete And Clear is not required, ok very well I don't see for load, store e […]
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #28 to this change.
sync: add Map.Clear
Fixes #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 77501052f421a502a9f8a0f2a04182bd33b3b465
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 136 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
5 comments:
File src/sync/map.go:
Patch Set #11, Line 164:
m.read.Store(new(readOnly))
I mean even why i commented the check for empty, tests were passing
The `if len(read.m) > 0 || read.amended {` check within the locked section avoids unnecessary allocations if multiple goroutines call `Clear` on a non-empty map simultaneously.
I would expect that to show up as a (moderate to small) difference in the number of allocations in `BenchmarkClear`, once the `sync.Mutex` is removed from that benchmark.
File src/sync/map_bench_test.go:
v1, ok := m.Load(k)
if !ok {
b.Logf("failed to load %v", k)
// b.Skip() Unsafe to call in perfG
continue
}
if v1.(int) != v {
b.Errorf("expected %v, got %v", k, v)
// b.Skip()
continue
}
}
ok, comment or remove the code. […]
I guess, but why?
The thing I hope to measure with this benchmark is the average rate of allocations per `Clear` call (discussed in https://go-review.git.corp.google.com/c/go/+/515015/11..28/src/sync/map.go#164).
Since that allocation occurs at the transition from non-empty to empty, reproducing it requires only `Store` and `Clear`.
File src/sync/map_bench_test.go:
Patch Set #28, Line 544:
mu.Lock()Again, please drop the mutex here — there is no need for it, and contention on the mutex will almost certainly bias the benchmark results by preventing multiple goroutines from running `Clear` concurrently.
// Skipping load calls due to concurrency
/*
v1, ok := m.Load(k)
if !ok {
b.Logf("failed to load %v", k)
// b.Skip() Unsafe to call in perfG
continue
}
if v1.(int) != v {
b.Errorf("expected %v, got %v", k, v)
// b.Skip()
continue
}*/
Don't leave commented-out code — it is unlikely that we would ever uncomment it, and not particularly essential to the benchmark (lots of real programs store to maps of monitoring data with ~0 loads in the steady state).
File src/sync/map_test.go:
Patch Set #28, Line 303:
tests the concurrent behavior of the sync.MapPlease make this comment shorter and more specific: it should focus on _what_ properties are being checked, not _how_ they are being checked. (Does this verify that concurrent calls to `Clear` and `Store` do not produce data races? Something else?)
The comment doesn't need to include the details about the specific goroutines, because we can read the code to figure that out if the test fails.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
2 comments:
File src/sync/map_test.go:
Patch Set #24, Line 364:
func TestConcurrentClear(t *testing.T) {Done
Done
File src/sync/map_test.go:
Patch Set #28, Line 303:
tests the concurrent behavior of the sync.Map
Please make this comment shorter and more specific: it should focus on _what_ properties are being c […]
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
File src/sync/map_bench_test.go:
Patch Set #28, Line 544:
mu.Lock()
Again, please drop the mutex here — there is no need for it, and contention on the mutex will almost […]
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #29 to this change.
sync: add Map.Clear
Fixes #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 61ecacc04e10111853bc7d4ed808c14df0222b68
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 130 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Patch Set #11, Line 164:
m.read.Store(new(readOnly))
The `if len(read.m) > 0 || read. […]
setup_call: go test -c -race
1. BenchmarkClear without Load Calls
i) i) with check for empty map in Clear
goos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 363613 2946 ns/op
BenchmarkClear/*sync_test.RWMutexMap
BenchmarkClear/*sync_test.RWMutexMap-8 525649 2621 ns/op
BenchmarkClear/*sync.Map
BenchmarkClear/*sync.Map-8 280063 7898 ns/op
PASSgoos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 467364 2432 ns/op
BenchmarkClear/*sync_test.RWMutexMap
BenchmarkClear/*sync_test.RWMutexMap-8 753368 1588 ns/op
BenchmarkClear/*sync.Map
BenchmarkClear/*sync.Map-8 388039 3165 ns/op
PASSgoos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 471136 2255 ns/op
BenchmarkClear/*sync_test.RWMutexMap
BenchmarkClear/*sync_test.RWMutexMap-8 663817 1596 ns/op
BenchmarkClear/*sync.Map
BenchmarkClear/*sync.Map-8 387192 3133 ns/op
PASSii) without check for empty map in Clear
goos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 471284 3348 ns/op
BenchmarkClear/*sync_test.RWMutexMap
BenchmarkClear/*sync_test.RWMutexMap-8 636710 1675 ns/op
BenchmarkClear/*sync.Map
BenchmarkClear/*sync.Map-8 388240 3132 ns/op
PASSgoos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 460369 2824 ns/op
BenchmarkClear/*sync_test.RWMutexMap
BenchmarkClear/*sync_test.RWMutexMap-8 703011 1611 ns/op
BenchmarkClear/*sync.Map
BenchmarkClear/*sync.Map-8 386132 3143 ns/op
PASSgoos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 462972 2674 ns/op
BenchmarkClear/*sync_test.RWMutexMap
BenchmarkClear/*sync_test.RWMutexMap-8 702590 1607 ns/op
BenchmarkClear/*sync.Map
BenchmarkClear/*sync.Map-8 389812 3117 ns/op
PASS
2. BenchmarkClear with Load Callsi) with check for empty map in Clear
goos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 393286 2641 ns/op
BenchmarkClear/*sync_test.RWMutexMap
BenchmarkClear/*sync_test.RWMutexMap-8 511614 2387 ns/op
BenchmarkClear/*sync.Map
BenchmarkClear/*sync.Map-8 249514 4807 ns/op
PASSgoos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 306735 3774 ns/op
BenchmarkClear/*sync_test.RWMutexMap
BenchmarkClear/*sync_test.RWMutexMap-8 411452 2948 ns/op
BenchmarkClear/*sync.Map
BenchmarkClear/*sync.Map-8 199868 6404 ns/op
PASS
ii) without check for empty map in Cleargoos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 450927 2620 ns/op
BenchmarkClear/*sync_test.RWMutexMap
BenchmarkClear/*sync_test.RWMutexMap-8 509013 2388 ns/op
BenchmarkClear/*sync.Map
BenchmarkClear/*sync.Map-8 249022 4768 ns/op
PASSgoos: darwin
goarch: arm64
pkg: sync
BenchmarkClear
BenchmarkClear/*sync_test.DeepCopyMap
BenchmarkClear/*sync_test.DeepCopyMap-8 458102 2637 ns/op
BenchmarkClear/*sync_test.RWMutexMap
BenchmarkClear/*sync_test.RWMutexMap-8 513540 2367 ns/op
BenchmarkClear/*sync.Map
BenchmarkClear/*sync.Map-8 250668 4850 ns/op
PASS
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
File src/sync/map_bench_test.go:
v1, ok := m.Load(k)
if !ok {
b.Logf("failed to load %v", k)
// b.Skip() Unsafe to call in perfG
continue
}
if v1.(int) != v {
b.Errorf("expected %v, got %v", k, v)
// b.Skip()
continue
}
}
I guess, but why? […]
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #30 to this change.
sync: add Map.Clear
Fixes #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 8ebf4c883cd4d88809391fb392799fd781620eaf
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 116 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Patch Set #11, Line 164:
m.read.Store(new(readOnly))
setup_call: go test -c -race […]
Done
File src/sync/map_bench_test.go:
// Skipping load calls due to concurrency
/*
v1, ok := m.Load(k)
if !ok {
b.Logf("failed to load %v", k)
// b.Skip() Unsafe to call in perfG
continue
}
if v1.(int) != v {
b.Errorf("expected %v, got %v", k, v)
// b.Skip()
continue
}*/
Don't leave commented-out code — it is unlikely that we would ever uncomment it, and not particularl […]
Done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #31 to this change.
sync: add Map.Clear
Fixes #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 80eb8f871fcdc4df1e185067b6e6bc9d3ec91340
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 115 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #32 to this change.
sync: add Map.Clear
Fixes #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: 43e80a0fc88a71f8b190923cad7bce291ad9aef6
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 115 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
qiulaidongfeng (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Patch set 32:Run-TryBot +1Code-Review +1
1 comment:
Patchset:
LGTM.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
1 comment:
Patchset:
1 of 46 TryBots failed. […]
why is this error poping up?
```
go: downloading github.com/BurntSushi/toml v1.2.1
../internal/versions/versions_go122.go:11:2: package go/version is not in std (/workdir/go/src/go/version)
Error: tests failed: exit status 1; exit status 1
```
the dep wasn't intro'ed by this CL
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
qiulaidongfeng (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Hiro Hamada, Ian Lance Taylor, Russ Cox.
Patch set 32:-Run-TryBot
1 comment:
Patchset:
why is this error poping up? […]
Run git codereview sync to fix it.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
Patchset:
Run git codereview sync to fix it.
ok. I need to update the branch i guess
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
1 comment:
Patchset:
ok. […]
done
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox, qiulaidongfeng.
Gerrit Bot uploaded patch set #33 to this change.
The following approvals got outdated and were removed: TryBot-Result-1 by Gopher Robot
sync: add Map.Clear
Fixes #61696
Change-Id: I0a31afd3bc433fc84280d56f2798bda10da61eba
GitHub-Last-Rev: c4237d7f09530c80cd8efe67de68bf84fb62c405
GitHub-Pull-Request: golang/go#61702
---
A api/next/61696.txt
M src/sync/map.go
M src/sync/map_bench_test.go
M src/sync/map_reference_test.go
M src/sync/map_test.go
5 files changed, 115 insertions(+), 1 deletion(-)
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
qiulaidongfeng (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Patch set 33:Run-TryBot +1Code-Review +1
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Hiro Hamada (Gerrit)
Attention is currently required from: Austin Clements, Bryan Mills, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Patch set 33:Code-Review +2Commit-Queue +1
Bryan Mills (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
Patch set 33:Auto-Submit +1
Bryan Mills (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Austin Clements, Dmitry Vyukov, Ian Lance Taylor, Russ Cox.
1 comment:
Patchset:
This missed the release window because I hadn't noticed it still had a Hold+1 vote from Russ pending proposal discussion. Since the proposal is now approved, I have removed the Hold+1; this should be good to go in when the window opens for Go 1.23.
To view, visit change 515015. To unsubscribe, or for help writing mail filters, visit settings.