[#1248] node: Do not update cache twice #30

carpawell · 2023-01-25T14:31:18Z

carpawell commented

2023-01-25 14:31:18 +00:00

(Migrated from github.com)

Adopted https://github.com/nspcc-dev/neofs-node/pull/2151.

ale64bit (Migrated from github.com) reviewed 2023-01-25 14:31:18 +00:00

dstepanov-yadro (Migrated from github.com) reviewed 2023-01-25 14:31:18 +00:00

fyrchik (Migrated from github.com) reviewed 2023-01-27 08:25:38 +00:00

cmd/frostfs-node/cache.go Outdated

					
				@ -29,1 +29,3 @@

					ttl time.Duration

					m       sync.RWMutex        // protects progMap

					progMap map[K]chan struct{} // contains fetch-in-progress keys

					ttl     time.Duration

fyrchik (Migrated from github.com) commented

2023-01-27 08:25:38 +00:00

We already have valueWithTime which can be extended. Can we avoid having a map and add a mutex to valueWithTime instead?

We already have `valueWithTime` which can be extended. Can we avoid having a map and add a mutex to `valueWithTime` instead?

carpawell (Migrated from github.com) reviewed 2023-02-20 12:28:42 +00:00

cmd/frostfs-node/cache.go Outdated

					
				@ -29,1 +29,3 @@

					ttl time.Duration

					m       sync.RWMutex        // protects progMap

					progMap map[K]chan struct{} // contains fetch-in-progress keys

					ttl     time.Duration

carpawell (Migrated from github.com) commented

2023-02-20 12:28:42 +00:00

what if multiple routines come and request the same value that does not exist yet?

acid-ant (Migrated from github.com) approved these changes 2023-02-28 11:26:01 +00:00

dstepanov-yadro (Migrated from github.com) reviewed 2023-03-01 08:21:44 +00:00

cmd/frostfs-node/cache.go Outdated

					
				@ -29,1 +29,3 @@

					ttl time.Duration

					m       sync.RWMutex        // protects progMap

					progMap map[K]chan struct{} // contains fetch-in-progress keys

					ttl     time.Duration

dstepanov-yadro (Migrated from github.com) commented

2023-03-01 08:21:44 +00:00

I think it's about something like this:
map[key]{*value, keyMutex}

First get object from this map with global mutex (it can be optimized by using hashed key and shards), then lock keyMutex to get value.

I think it's about something like this: map[key]{*value, keyMutex} First get object from this map with global mutex (it can be optimized by using hashed key and shards), then lock keyMutex to get value.

carpawell commented

2023-03-22 17:39:47 +00:00

@fyrchik, @dstepanov-yadro, lets continue that.

Can we avoid having a map and add a mutex to valueWithTime instead?

@fyrchik, the whole PR is useless then and no new optimization will be merged.

I think it's about something like this:
map[key]{*value, keyMutex}

@dstepanov-yadro, updated PR, check, please. i think, i may get you wrong.

@fyrchik, @dstepanov-yadro, lets continue [that](https://github.com/TrueCloudLab/frostfs-node/pull/30#discussion_r1088691590). > Can we avoid having a map and add a mutex to valueWithTime instead? @fyrchik, the whole PR is useless then and no new optimization will be merged. > I think it's about something like this: map[key]{*value, keyMutex} @dstepanov-yadro, updated PR, check, please. i think, i may get you wrong.

carpawell added the due date 2023-03-24

2023-03-22 17:39:59 +00:00

dstepanov-yadro requested changes 2023-03-23 06:14:21 +00:00

cmd/frostfs-node/cache.go Outdated

					
				@ -48,3 +59,4 @@

					}

				}

				func (c *ttlNetCache[K, V]) actualValue(val any) (*valueWithTime[V], bool) {

dstepanov-yadro commented

2023-03-23 06:09:16 +00:00

can we replace any with generic type?

can we replace ```any``` with generic type?

carpawell commented

2023-03-23 16:27:33 +00:00

hm, yeah, actualValue is useless with generic, in fact. dropped

hm, yeah, `actualValue` is useless with generic, in fact. dropped

cmd/frostfs-node/cache.go Outdated

					
				@ -51,0 +68,4 @@

					return nil, false

				}

				func (c *ttlNetCache[K, V]) waitForUpdate(key K, vip *valueInProgress[V]) (V, error) {

dstepanov-yadro commented

2023-03-23 06:06:48 +00:00

key is redundant

fyrchik commented

2023-03-23 06:18:33 +00:00

Also, why is it a method of ttlNetCache?

Also, why is it a method of `ttlNetCache`?

carpawell commented

2023-03-23 16:28:03 +00:00

used to be used, dropped

ale64bit reviewed 2023-03-23 09:19:38 +00:00

cmd/frostfs-node/cache.go Outdated

					
				@ -27,0 +29,4 @@

				type valueInProgress[V any] struct {

					v V

					e error

					m sync.RWMutex

ale64bit commented

2023-03-23 09:11:30 +00:00

not sure if it's a common practice, but I normally see the mutex declared right before the thing it guards.

carpawell commented

2023-03-23 16:28:29 +00:00

do you mean field order? (if yes, fixed)

👍 1

ale64bit marked this conversation as resolved

cmd/frostfs-node/cache.go Outdated

					
				@ -28,2 +36,3 @@

				type ttlNetCache[K comparable, V any] struct {

					ttl time.Duration

					m       sync.RWMutex              // protects progMap

					progMap map[K]*valueInProgress[V] // contains fetch-in-progress keys

ale64bit commented

2023-03-23 09:16:15 +00:00

is it simpler to use https://pkg.go.dev/sync#Map and avoid all the mutexes?

dstepanov-yadro commented

2023-03-23 09:25:53 +00:00

The Map type is specialized. Most code should use a plain Go map instead, with separate locking or coordination, for better type safety and to make it easier to maintain other invariants along with the map content.

> The Map type is specialized. Most code should use a plain Go map instead, with separate locking or coordination, for better type safety and to make it easier to maintain other invariants along with the map content.

ale64bit commented

2023-03-23 09:28:54 +00:00

Sure, but this usage seems to fall into the description on the next paragraph after the one you are quoting, isn't? And type safety is a non-concern given that the parent type is polymorphic.

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.

Sure, but this usage seems to fall into the description on the next paragraph after the one you are quoting, isn't? And type safety is a non-concern given that the parent type is polymorphic. > > The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.

carpawell commented

2023-03-23 16:29:47 +00:00

did not think about it cause i've never used it and we do not have it in our repos. added an exapmle (separate commit), may be squashed or dropped @fyrchik @acid-ant @aarifullin

dstepanov-yadro commented

2023-03-24 05:40:21 +00:00

(1) when the entry for a given key is only ever written once but read many times, as in caches that only grow

There are delete calls exists.

(2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys.

This point is closer to how this cache will be used, but does not fully correspond to it.

> (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow There are `delete` calls exists. > (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. This point is closer to how this cache will be used, but does not fully correspond to it.

👍 1

carpawell commented

2023-03-24 09:46:31 +00:00

ok, dropped

carpawell force-pushed carpawell/fix/multiple-cache-update-requests-FROST from 34c43b18a7 to 0a01917d83

2023-03-23 16:32:33 +00:00

Compare

carpawell requested review from dstepanov-yadro 2023-03-23 16:32:43 +00:00

dstepanov-yadro approved these changes 2023-03-24 05:41:26 +00:00

ale64bit reviewed 2023-03-24 06:55:03 +00:00

cmd/frostfs-node/cache.go Outdated

					
				@ -64,3 +80,3 @@

					}

					v, err := c.netRdr(key)

					valInProg := &valueInProgress[V]{}

ale64bit commented

2023-03-24 06:55:03 +00:00

if you happen to keep this version, it probably should be

val, ok := c.progMap.LoadOrStore(key, &valueInProgress[V]{})
val.m.Lock()
defer val.m.Unlock()
if ok {
(...)

if you happen to keep this version, it probably should be ``` val, ok := c.progMap.LoadOrStore(key, &valueInProgress[V]{}) val.m.Lock() defer val.m.Unlock() if ok { (...) ```

carpawell commented

2023-03-24 09:39:59 +00:00

well, it seems like it is wrong to me: the whole PR is about the restricting simultaneous network cache updates so we need an "updating" routine to fill map with already locked mutex, otherwise, other routines are able to get it and RLock its mutex despite the fact that the value is not fetched yet

so i think it should be either LoadOrStore with already held mutex or Load + Store if we dont want to have useless mutexes

well, it seems like it is wrong to me: the whole PR is about the restricting simultaneous network cache updates so we _need_ an "updating" routine to fill map with already locked mutex, otherwise, other routines are able to get it and `RLock` its mutex despite the fact that the value is not fetched yet so i think it should be either `LoadOrStore` with already held mutex or `Load` + `Store` if we dont want to have useless mutexes

ale64bit commented

2023-03-24 09:48:13 +00:00

I see. But what I had in mind is that if you are calling LoadOrStore and there happened to be a value already (locked or otherwise), the mutex you just created and locked is never actually unlocked, is it? (the one from the fresh val that didn't happen to be stored in the map).

I see. But what I had in mind is that if you are calling `LoadOrStore` and there happened to be a value already (locked or otherwise), the mutex you just created and locked is never actually unlocked, is it? (the one from the fresh `val` that didn't happen to be stored in the map).

carpawell force-pushed carpawell/fix/multiple-cache-update-requests-FROST from 0a01917d83 to 261bf48b97

2023-03-24 09:47:48 +00:00

Compare

carpawell force-pushed carpawell/fix/multiple-cache-update-requests-FROST from 261bf48b97 to dfa39203e8

2023-03-30 16:09:16 +00:00

Compare

fyrchik reviewed 2023-03-30 19:08:25 +00:00

cmd/frostfs-node/cache.go Outdated

					
				@ -70,0 +110,4 @@

					valInProg.m.Unlock()

					c.m.Lock()

					delete(c.progMap, key)

fyrchik commented

2023-03-30 19:08:04 +00:00

Why do we unconditionally delete something from the map we have just written to?

carpawell commented

2023-04-18 17:03:20 +00:00

if we are here (that line), we are the routine (the only one per a value) that failed to get a val from the cache and decided to fetch if from the network. when the work is done, we remove record from "the scheduler" (progMap) cause it is finished

if we are here (that line), we are the routine (the only one per a value) that failed to get a val from the cache and decided to fetch if from the network. when the work is done, we remove record from "the scheduler" (`progMap`) cause it is finished

fyrchik marked this conversation as resolved

fyrchik reviewed 2023-03-30 19:09:25 +00:00

cmd/frostfs-node/cache.go

					
				@ -68,1 +103,4 @@

					c.m.Unlock()

					v, err := c.netRdr(key)

					c.set(key, v, err)

fyrchik commented

2023-03-30 19:09:25 +00:00

Do we still need this line?

carpawell commented

2023-04-18 16:59:41 +00:00

yes, that's a cache still. why did you doubt? got the value (not concurrently with that PR) -- updated the cache

fyrchik commented

2023-04-19 10:48:24 +00:00

Because below we explicitly set valInProg values

Because below we explicitly set `valInProg` values

carpawell commented

2023-04-19 14:22:42 +00:00

below we explicitly set valInProg values

it is for a concurrent routines (that wait when network job is done) only. all the other will take a val directly from the cache cause no cache miss will be faced after an update (well, for 30s only but anyway)

> below we explicitly set valInProg values it is for a concurrent routines (that wait when network job is done) only. all the other will take a val directly from the cache cause no cache miss will be faced after an update (well, for 30s only but anyway)