2

Ok, for some simplified setup, in this example we have three structs comp, agg and cache that look somewhat like this:

type comp struct {
    id  uint64
    val float64
}

type agg struct {
    id   uint64
    vals []*comp
}

type cache struct {
    compMtx sync.RWMutex
    comps map[uint64]*comp
    aggMtx sync.RWMutex
    aggs  map[uint64]*agg
}

Where the cache has the following function to add new comp values, which in the case of the update does not seem to work:

func (c *cache) NewComp(cpNew *comp) {
    compMtx.Lock()
    defer compMtx.Unlock()
    cpOld, ok := c.comps[cpNew.id]
    if ok { // update
        addr := &cpOld // this is of type **comp
        *addr = cpNew
    } else { // new value
        c.comps[cpNew.id] = cpNew
    }
}

The thought behind this method is that by changing where the pointer of the pointer points to, we can ensure that the comp pointers in agg.vals always point to the newest iteration of a given comp object.

As for the reason behind this approach, iterating over the entirety of an agg.vals array to find the index of the given comp object would a) be computationally expensive due to the (rather large) size of the array and would b) require agg to be locked via an internal sync.Mutex during the search to block different threads from accessing the object, both of which are undesirable. Furthermore assume that making agg.value into a map so as to facilitate easy updates is not a possibility.

Since NewComp as implemented above does however not work, my question would be whether there are any obvious mistakes in the function above, or if I made some fundamental error in my thinking here?


As this might be helpful, here also an example in which the update via pointers of pointers works as expected:

type wrks struct {
    id   uint64
    val1 *comp
    val2 *comp
}

func compute() *comp {...}

func updateComp(c **comp) {
    *c = compute()
}

func processObj(o *obj) {
    updateComp(&o.val1)
    updateComp(&o.val2)
}

I fail to see the fundamental difference between the two, but I may have been staring at this for too long at this point.

icza
  • 389,944
  • 63
  • 907
  • 827
abcalphabet
  • 1,158
  • 3
  • 16
  • 31

1 Answers1

1

You store pointers in the map, so when you get a pointer from it, just modify the pointed value, assign to the pointed value:

cpOld, ok := c.comps[cpNew.id]
if ok { // update
    *cpOld = *cpNew
} else { // new value
    c.comps[cpNew.id] = cpNew
}

See a simplified example on the Go Playground that shows this works.

Your original code doesn't work because cpOld is a pointer, but it's a copy of the pointer stored in the map. If you modify the value of this cpOld variable, that has no effect on the value stored in the map. You can't change values in a map, you can only reassign / store new values in them.

One thing you should keep in mind: the cpNew pointer passed to NewComp() will not be "useful" after the update, because we didn't use the pointer, we just used the pointed value to update the value that was already pointed by the map (a value stored in the map). If you want the pointer to keep pointing to the same value, you have no other choice but to store that pointer in the map, like if it was new:

cpOld := c.comps[cpNew.id] = cpNew

See related question: How to update map values in Go

icza
  • 389,944
  • 63
  • 907
  • 827
  • I see, updating via `*cpOld = *cpNew` also does not do the trick sadly. I gave it try earlier, probably should have mentioned that. – abcalphabet Oct 15 '20 at 21:07
  • Hmm, thanks, so if the pointer is always a copy of the pointer within the map, would storing the pointer of the pointer in a map and reassigning from the copy of `**comp` work then? – abcalphabet Oct 15 '20 at 21:09
  • But it works, see a simplified example on the [Go Playground](https://play.golang.org/p/kgPEQXQEF8J). – icza Oct 15 '20 at 21:12
  • Indexing the map will give you a copy of the value stored in the map. You store that copy in `cpOld`, but `cpOld` does not share memory with the value stored in the map. No matter what you do with your `cpOld` variable, that will not change the value stored in the map. – icza Oct 15 '20 at 21:14
  • Oh yeah, in the playground it does indeed work as expected. Very odd that it doesn't work in my case then it seems I have an error elsewhere that I have to investigate... – abcalphabet Oct 15 '20 at 21:25
  • Go beginner here and I doubt if we can just use `c.comps[cpNew.id] = cpNew` to avoid key existence testing? I see no difference [Go Playground](https://play.golang.org/p/Gf4JZ4UcVKt) – lincr Oct 16 '20 at 02:36
  • @lincr The difference is detailed in the answer. If you just store the new value using `c.comps[cpNew.id] = cpNew`, then the pointed passed to `NewComp()` will continue to point to the same component. If you check for existence and assign to the pointed value, the passed pointer will no longer point to the same component. – icza Oct 16 '20 at 07:59
  • Sorry actually I didn't get the idea of the last paragraph in your answer... You say `If you check for existence and assign to the pointed value, the passed pointer will no longer point to the same component. ` but how? Let's say `id = 1`, previously id 1 points to `0xA0` (an example address), then we use `cpNew` which points to `0xB0` to update the map, why after that cpNew will not still point to `0xB0`? – lincr Oct 16 '20 at 08:46
  • @lincr If after the `NewComp()` call you modifie the passed component, it will have no effect on the component pointed by the map. See here: [Go Playground](https://play.golang.org/p/ppSCPvkEBIP). But if you always assign, then modifying after will change the component pointed by the map, see here: [Go Playground](https://play.golang.org/p/Cs8o8sQW1_I). – icza Oct 16 '20 at 08:49
  • I have C and pointer backgrounds. I think I somehow misunderstood your idea. You are talking about the pointer which is the value of the map before and after assignment/update. But I wrongly think you are talking about the pointer which is the argument passed in. I think always using pointer assignment is also OK if you intend to do that. – lincr Oct 16 '20 at 09:02