fishy commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r779873522
##########
File path: lib/go/thrift/simple_server.go
##########
@@ -193,7 +195,13 @@ func (p *TSimpleServer) innerAccept() (int32, error) {
}
if client != nil {
p.wg.Add(1)
+ p.clients[client] = struct{}{}
go func() {
+ defer func() {
+ p.mu.Lock()
+ defer p.mu.Unlock()
+ delete(p.clients, client)
Review comment:
Actually, I misinterpreted that go runtime bug. It's just that the
capacity of the map only grows and never shrinks (so delete won't cause the map
to shrink), but memory holding deleted items can be reused for future inserts.
So although this is still unbounded, it will be more like a high watermark of
how many concurrent connection the server handles, and unlikely to cause OOM.
So using a map is fine. But we still need to handle the map key issue.
This is the code I used to verify the behavior (note the different
allocations between the 2 benchmark tests):
```
$ go test -v -bench .
=== RUN TestHelpers
main_test.go:43: size: 1000
main_test.go:46: size: 502
--- PASS: TestHelpers (0.00s)
goos: linux
goarch: amd64
pkg: test
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkMap
BenchmarkMap/fill-twice
main_test.go:60: total size 2000, n 1, average size: 2000
main_test.go:60: total size 200000, n 100, average size: 2000
main_test.go:60: total size 5982000, n 2991, average size: 2000
main_test.go:60: total size 14522000, n 7261, average size: 2000
BenchmarkMap/fill-twice-12 7261 184987 ns/op
101042 B/op 113 allocs/op
BenchmarkMap/fill-delete-fill
main_test.go:72: total size 1505, n 1, average size: 1505
main_test.go:72: total size 150243, n 100, average size: 1502
main_test.go:72: total size 8163265, n 5442, average size: 1500
main_test.go:72: total size 12801894, n 8534, average size: 1500
BenchmarkMap/fill-delete-fill-12 8534 146318 ns/op
50348 B/op 91 allocs/op
PASS
ok test 3.927s
$ cat main_test.go
package main
import (
"math/rand"
"os"
"testing"
)
const (
n = 1000
margin = 0.1
)
type mapType = map[int]struct{}
func fillMap(m mapType, n int) {
for i := 0; i < n; i++ {
m[rand.Int()] = struct{}{}
}
}
func deleteHalf(m mapType) {
for k := range m {
if rand.Float64() >= 0.5 {
delete(m, k)
}
}
}
func compareMargin(tb testing.TB, want, got int, margin float64) {
tb.Helper()
diff := int(float64(want) * margin)
if got < want-diff || got > want+diff {
tb.Errorf("Want %d got %d margin %v", want, got, margin)
}
}
func TestHelpers(t *testing.T) {
m := make(mapType)
fillMap(m, n)
t.Logf("size: %d", len(m))
compareMargin(t, n, len(m), margin)
deleteHalf(m)
t.Logf("size: %d", len(m))
compareMargin(t, n/2, len(m), margin)
}
func BenchmarkMap(b *testing.B) {
b.Run("fill-twice", func(b *testing.B) {
b.ReportAllocs()
var totalSize int
for i := 0; i < b.N; i++ {
m := make(mapType)
fillMap(m, n)
fillMap(m, n)
totalSize += len(m)
}
b.Logf("total size %d, n %d, average size: %v", totalSize,
b.N, totalSize/b.N)
})
b.Run("fill-delete-fill", func(b *testing.B) {
b.ReportAllocs()
var totalSize int
for i := 0; i < b.N; i++ {
m := make(mapType)
fillMap(m, n)
deleteHalf(m)
fillMap(m, n)
totalSize += len(m)
}
b.Logf("total size %d, n %d, average size: %v", totalSize,
b.N, totalSize/b.N)
})
}
func TestMain(m *testing.M) {
rand.Seed(1)
os.Exit(m.Run())
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
