Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Publish call high latency #494

Open
aratz-lasa opened this issue Jul 19, 2022 · 7 comments
Open

Publish call high latency #494

aratz-lasa opened this issue Jul 19, 2022 · 7 comments

Comments

@aratz-lasa
Copy link

Hello,

The Publish call, in my application, is taking in average 5ms to complete. Moreover, it makes use of an internal lock. Therefore, if you execute Publish N times in parallel, it takes 5ms+N to complete all of them.

Taking this into account, I have a question: What is the lock used for, apart for accessing the t.closed bool? Is there a way to get rid of it? Right now the Publish call it is a huge bottleneck.

@vyzo
Copy link
Collaborator

vyzo commented Jul 20, 2022

That's unfortunate.

Are you using the topic publish method or the pubsub level Publish method?

Can you profile this?
Also, can you check with 0.6/0.7?
Lets see if there is a regression.

@aratz-lasa
Copy link
Author

aratz-lasa commented Jul 20, 2022

First of all, thank you for your fast response!

I have profiled it and the most time takes it at signing messages. If I disable signing it reduces from 5ms to 0.3ms. So I guess it's not bad. However, I do want to sign the messages. Moreover, there is no really a regression from 0.6 to 0.7.

However, I would like to know, what is the lock needed for?

@vyzo
Copy link
Collaborator

vyzo commented Jul 21, 2022

Are you using RSA keys for some reason?

Can you try with ed25529 keys?
It should be considerably faster.

@vyzo
Copy link
Collaborator

vyzo commented Jul 21, 2022

The lock is used to prevent data races, maybe we can get rid of it indeed.

@aratz-lasa
Copy link
Author

aratz-lasa commented Jul 21, 2022

I checked and we do use RSA, because when you create a new Host in libp2p, if you don't specify the Identity option it fallbacks into RSA. So I will change that, thanks!

On the other hand, what is the data race that we prevent using the lock? I checked and I could not find any, apart from the closed boolean, which can be replaced by a uber atomic.bool.

@vyzo
Copy link
Collaborator

vyzo commented Jul 21, 2022

Or a CAS with sync/atomic; care for a patch?

@aratz-lasa
Copy link
Author

Okay, patch done. I used the uber package since internally makes use of uint32 and the sync.atomic operations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants