Rework clientmap.RefcountMap, add tests, add bench tests.

[Bolodya1997]

Issue

Existing refcount map is incorrect, such test can fail:

var m clientmap.RefcountMap
m.Store("a", null.NewClient())

var loaded bool
go func() {
    _, loaded = m.Load("a") // count += 1 (if exists)
}()
go m.Delete("a") // count -= 1 (delete if 0)

_, exists := m.Load("a")
require.Equal(t, loaded, exists) // if loaded, Delete can't delete, only should decrement the count

Solution

Rework refcount map with per-entry locks, add action tests, add bench tests.

[denis-tingaikin]

@Bolodya1997

Existing refcount map is incorrect, such test can fail:

var loaded bool
go func() {
_, loaded = m.Load("a") // count += 1 (if exists)
}()

Is test failing if work with loaded flag synchronized?

[Bolodya1997]

Is test failing if work with loaded flag synchronized?

Yep, it is not the line to line test. In the real test there was sync.WaitGroup additionally used.

[denis-tingaikin]

Why do we need to use 1000 here?

[Bolodya1997]

You are right, there is no need to use such big numbers in here. Our case is:

• executors count > IDs count (multiple connections to single clientURL)
• IDs count and executors count should be some prime numbers to be sure that all types of operations are executed on all ID+executor pairs

Probably something like 23 executors and 11 IDs is well enough for us in such case.

[denis-tingaikin]

This allocation could a little bit affects results.

[Bolodya1997]

I have tried with it and without it, it doesn't affect the results.

[denis-tingaikin]

I guess it can affect tests with cmd go test -v -bench=. -benchmem

[Bolodya1997]

Fixed with per-test readonly client.

[denis-tingaikin]

Why 1000?

[Bolodya1997]

My fault it is not actually the parallelCount but the retriesCount.
TestRefcountMap_Actions is not the test fully validating the correctness of the RefcountMap. It should eventually fail if RefcountMap is invalid. So if we start increasing retries count, such eventually is becoming most probably.
1000 is good, because it is not too big to make test performing > 1 sec (10000 is too big for such case) and not too small to keep eventually somewhere far away from us.

[denis-tingaikin]

I feel this test can be simplified.

Please take a look at tests from sync.Map https://golang.org/src/sync/map_test.go

[Bolodya1997]

It is the only way to add some eventually failing on incorrect implementation test I see. There are no such tests actually in the sync.Map tests. Such tests should be very helpful for someone who will try to optimize the RefcountMap implementation.