search

libp2p / go-libp2p-pubsub

The PubSub implementation for go-libp2p
https://github.com/libp2p/specs/tree/master/pubsub
Other
323 stars 187 forks source link

Replaced topic lock by atomic boolean to avoid lock contention #495

Open aratz-lasa opened 2 years ago

aratz-lasa commented 2 years ago

PR opened in response to #494.

In the topic struct, there is currently lock contention due to the internal lock that is used to check if the topic has been closed or not. This generates lock contention, in those cases where the publish call takes a long time, such as if messages are signed with RSA.

Therefore, in this PR is proposed to replace the lock with an atomic boolean, allowing concurrent calls to Publish, and relieving lock contention.

aratz-lasa commented 2 years ago

I ran gofmt to all the source code and pushed again.

Lock contention occurs because the pubsub producers go faster than the time it takes to sign with Ed25519, therefore, they end up queueing up, since they cannot enter the publish call until the lock is free.

What do you mean by "it's not entirely the same"?

vyzo commented 2 years ago

its an RLock, so they should not content, they can hold it concurrently. Only close takes the exclusive lock.

It is not entirely the same because an active publish would prevent the topic from being closed concurrently.

vyzo commented 2 years ago

Let me think about it a bit.

I dont think there will be any deleterious effects from the change, but it is still a change in behaviour.

aratz-lasa commented 2 years ago

Aaah you are right! I thought that the RLock could only be acquired by goroutines in the same thread (since that's how it works in other languages). However I looked it up and I was wrong. There can be concurrent publish calls despite the goroutines being in different threads. So it's your call!