segment the memory peerstore + granular locks #78
Conversation
ghost
assigned 
|
Just a summary of progress: this patch, together with libp2p/go-libp2p-connmgr#40, has eliminated all lock contention in the test relay. |
|
I will move the peerstore striping logic to its own component, and add support for the ds-backed peerstore as well. |
|
Split off the protocol book in the interface, with implementations for both the memory and datastore-backed peerstore. |
|
this is ready for review now. |
pstoremem/protobook.go
Outdated
|
|
||
| func (s *protoSegments) get(p peer.ID) *protoSegment { | ||
| b := []byte(p) | ||
| return s[b[len(b)-1]] |
There was a problem hiding this comment.
Go may optimize this, but it might just decide to copy the slice. How about s[byte(p[len(p)-1])]?
There was a problem hiding this comment.
hrm, I don't think that it copies in such casts, but hard to be certain.
There was a problem hiding this comment.
How does the rune to byte conversion work here btw?
There was a problem hiding this comment.
hrm, I don't think that it copies in such casts, but hard to be certain.
It []byte(string) usually copies as []byte is mutable whyle strings aren't. However, go does the right thing in some cases, IIRC.
How does the rune to byte conversion work here btw?
Go indexes strings by byte. Think of go strings like python strings, they can contain arbitrary bytes but are sometimes valid UTF8.
There was a problem hiding this comment.
ok, avoided the intermediate byte slice.
pstoremem/addr_book.go
Outdated
| } | ||
|
|
||
| func (mab *memoryAddrBook) PeersWithAddrs() peer.IDSlice { | ||
| mab.addrmu.RLock() | ||
| defer mab.addrmu.RUnlock() | ||
| var length uint32 |
There was a problem hiding this comment.
Optimizing for this case probably isn't worth it. This function isn't likely to be called frequently so we can probably just allocate the peer ID slice as needed and drop the atomic length updating logic.
pstoreds/protobook.go
Outdated
|
|
||
| func (pb *dsProtoBook) SetProtocols(p peer.ID, protos ...string) error { | ||
| pb.Lock(p) | ||
| defer pb.Unlock(p) |
There was a problem hiding this comment.
We could also just require a transaction datastore and avoid locking entirely. cc @raulk?
pstoreds/protobook.go
Outdated
| return &dsProtoBook{meta: meta} | ||
| } | ||
|
|
||
| func (pb *dsProtoBook) Lock(p peer.ID) { |
There was a problem hiding this comment.
These methods should be private.
pstoreds/protobook.go
Outdated
| } | ||
|
|
||
| func (pb *dsProtoBook) lock(p peer.ID) { | ||
| pb.lks[byte(p[len(p)-1])].Lock() |
There was a problem hiding this comment.
nit: a segment(p peer.ID) *segment function would be nice (could even drop all of these functions and just call pb.segment(p).Lock().
There was a problem hiding this comment.
ok, will do. I'll call it get similar to how we do it in the memory peerstore.
Quick attempt to remove the global lock on addr map. Should reduce lock contention under high load. Please try out and report results, @vyzo!