Gotomic hash and race conditions

Some time ago, Matt pointed out that there are race conditions in package “query”.

As the others have pointed out, this is coming from gotomic hash. If we run just one test in the gotomic package, the race detector will lit up:

go test -race --run TestConcurrency

I suspect this is a red herring, and will look into how lock-free hash maps and how go race detector work tomorrow.

Note: The four packages that fail are query, commit, cmd/dgraph, cmd/dgraphloader.

Update: gotomic has been updated and the above test (TestConcurrency) now runs fine.

@Wei had been looking into replacing zond/gotomic. That package is also not optimized for performance, and is considering more expensive both in terms of RAM and CPU.

This is also one of the projects that needs work – to implement a lock-free hash map, which can support, Get, Put and Iterate. The challenge here is growth. zotomic is based on an interesting paper which uses Linked List for growth. From what I understand, there is a newer paper which claims to be faster. So, definitely worth researching and implementing.

I was thinking whether we could have some interface for this hash map, and then we can swap the implementations underneath.

And for testing, we can swap this out for one that will lock the whole map for every access. Usually if we do this, there might be false negatives for the race detector, but in this case, a lock-free map is supposed to work without the user having to lock explicitly, so I don’t think we need to worry about false negatives.

What do you all think? @core-devs

That was how the previous implementation worked, and it was really slow. I tried one lock, then tried 32 locks with 32 buckets in the map, and they both tanked compared to the implemented lockless map. I don’t think we should switch until we have a solid replacement for gotomic.

I’m assuming that Go’s race detector can work fine with atomics. So, if it complains about a race condition in gotomic, it’s at least worth filing a bug against them, while we figure out whether we should be implementing our own.

The lock whole map idea is just for testing with race detector, not for the actual code. The motivation is that we still want to detect race conditions, but not those due to the lock-free hash map.

Ok, I will file a bug for them.

Ah… I see what you mean. That sounds like a good idea.

This topic was automatically closed 30 days after the last reply. New replies are no longer allowed.