question: should tm2/pkg/cmap be deprecated as it predates Go's standard library sync.Map

[odeke-em]

For the beta and mainnet launch, per the call for audits it has been requested that tm2 also be audited but while doing so I noticed this code https://github.com/gnolang/gno/blob/master/tm2/pkg/cmap/cmap.go in which really it is just a map with a mutex but doesn't account for load characteristics and content the way that Go's standard library https://pkg.go.dev/sync#Map

Perhaps let us delete the code in cmap and instead use sync.Map

Kindly paging @n2p5 @moul @petar-dambovaliev

[thehowl]

Agreed, let's change it to a sync.Map

[odeke-em]

@thehowl sadly the results show that sync.Map ain't any better and is much more of a memory hog

$ benchstat before.txt after.txt 
name                               old time/op    new time/op    delta
CMapConcurrentInsertsDeletesHas-8     1.72s ±11%     1.92s ± 3%   +11.66%  (p=0.000 n=10+9)
CMapHas-8                             109ns ± 9%     118ns ± 3%    +8.26%  (p=0.002 n=10+8)

name                               old alloc/op   new alloc/op   delta
CMapConcurrentInsertsDeletesHas-8    1.18GB ± 2%    3.21GB ± 3%  +172.09%  (p=0.000 n=10+10)
CMapHas-8                             16.0B ± 0%     16.0B ± 0%      ~     (all equal)

name                               old allocs/op  new allocs/op  delta
CMapConcurrentInsertsDeletesHas-8      824k ± 0%     4433k ± 0%  +437.89%  (p=0.000 n=10+10)
CMapHas-8                              2.00 ± 0%      1.60 ±38%      ~     (p=0.065 n=9+10)

given these benchmarks

var sink any = nil

func BenchmarkCMapConcurrentInsertsDeletesHas(b *testing.B) {
        cm := NewCMap()
        keys := make([]string, 100000)
        for i := range keys {
                keys[i] = fmt.Sprintf("key%d", i)
        }
        b.ResetTimer()

        for i := 0; i < b.N; i++ {
                var wg sync.WaitGroup
                semaCh := make(chan bool)
                nCPU := runtime.NumCPU()
                for j := 0; j < nCPU; j++ {
                        wg.Add(1)
                        go func() {
                                defer wg.Done()

                                // Make sure that all the goroutines run at the
                                // exact same time for true concurrent tests.
                                <-semaCh

                                for i, key := range keys {
                                        if (j+i)%2 == 0 {
                                                cm.Has(key)
                                        } else {
                                                cm.Set(key, j)
                                        }
                                        _ = cm.Size()
                                        if (i+1)%3 == 0 {
                                                cm.Delete(key)
                                        }

                                        if (i+1)%327 == 0 {
                                                cm.Clear()
                                        }
                                        _ = cm.Size()
                                        _ = cm.Keys()
                                }
                                _ = cm.Values()
                        }()
                }
                close(semaCh)
                wg.Wait()

                sink = wg
        }

        if sink == nil {
                b.Fatal("Benchmark did not run!")
        }
        sink = nil
}

and this code

//go:build fresh
package cmap

import (
	"sync"
)

// CMap is a goroutine-safe map
type CMap struct {
	m *sync.Map
}

func NewCMap() *CMap {
	return &CMap{
		m: new(sync.Map),
	}
}

func (cm *CMap) Set(key string, value interface{}) {
	cm.m.Store(key, value)
}

func (cm *CMap) Get(key string) interface{} {
	val, _ := cm.m.Load(key)
	return val
}

func (cm *CMap) Has(key string) bool {
	_, ok := cm.m.Load(key)
	return ok
}

func (cm *CMap) Delete(key string) {
	cm.m.Delete(key)
}

func (cm *CMap) Size() (size int) {
	cm.m.Range(func(_, _ any) bool {
            size++
            return true
        })
	return size
}

func (cm *CMap) Clear() {
	cm.m.Clear()
}

func (cm *CMap) Keys() (key []string) {
	keys := make([]string, 0, 10)
	cm.m.Range(func(k, _ any) bool {
		keys = append(keys, k.(string))
		return true
	})
	return keys
}

func (cm *CMap) Values() []interface{} {
	values := make([]any, 0, 10)
	cm.m.Range(func(_, value any) bool {
		values = append(values, value)
		return true
	})
	return values
}

And the results now make sense to me, sync.Map uses more than 1 map internally, performs a bunch of copying; I'll later on raise an issue with the Go programming language for fixes and also to expose a .Len() method too.

I'll close this issue, but I'll send a PR for benchmarks.

[odeke-em]

I have sent the PR #3540 with a cautionary tale and benchmark to ensure that future changes get quantified.

[thehowl]

Just a note, it may make sense to test this out with supernova rather than just doing a microbenchmark, just because I'm sure sync.map is faster in some specific cases. but we need to test it in a real context to be sure

[odeke-em]

@thehowl hehe right? So that's the beauty of fallacies until busted; even the same problems are exhibited when people try to use sync.RWMutex with RLock() presuming it'll be faster than sync.Mutex.Lock() et al for which we now just encourage people to simply use sync.Mutex because it works much better under load for less allocations plus CPU time can be tolerated https://gist.github.com/odeke-em/234bc88fbaf9e439b70a618ce6674659. Even this situation is hypothetically good when modelled but the situation I showed isn't even in benchmarks in the Go standard library, which I'll add sometime when I have the time.

Cool, I didn't know about the stress tester supernova, would be good indeed to test it out.