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

perf/basic_host: Don't handle address change if we hasn't anyone #1115

Merged
merged 2 commits into from
Jun 30, 2021

Conversation

snyh
Copy link
Contributor

@snyh snyh commented Jun 4, 2021

basic_host.background is used for handling address changed events, but the node may be created with libp2p.NoListenerAddrs.

the h.updateLocalIpAddr is cost a little CPU time.

But I use NoListenerAddrs with many (one thousand) tiny test nodes, and all these test nodes communicate with other peers by a full feature node (by pubsub or relay) in the same process.

The basic_host.background would use many CPU time and add 1000 useless goroutine in this situatuion.


I'm not sure whether the network could do listen after host.background started.
So I just keep the background goroutine and skip one round if there hasn't any listener address.

@snyh
Copy link
Contributor Author

snyh commented Jun 4, 2021

@Stebalien thanks for #398

@Stebalien
Copy link
Member

Is it the call to Addrs or the call to update the IP address that costs CPU? Could you post a CPU profile?

Notes:

  1. We could easily make Addrs short-circuit if we're not actually listening on any transports. That should save a fair amount.
  2. We could avoid updating the local IP address... I'm just slightly concerned about stale addresses and want to see if we can just optimize it.

@snyh
Copy link
Contributor Author

snyh commented Jun 7, 2021

1.pb.gz
Screenshot from 2021-06-07 09-10-04

profile with 30s before this PR

@snyh
Copy link
Contributor Author

snyh commented Jun 7, 2021

Screenshot from 2021-06-07 09-12-49
2.pb.gz

after this PR

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a lot of head scratching to make sure this can't go wrong... I'm convinced it can't go wrong.

That way, `Addrs` and the addresses emitted on the event bus are always
consistent.
@Stebalien
Copy link
Member

Well, one small change. We want Addrs and the emitted events to be the same.

@Stebalien Stebalien merged commit e5d5d44 into libp2p:master Jun 30, 2021
@Stebalien
Copy link
Member

And thanks for the detailed perf analysis!

@aschmahmann aschmahmann mentioned this pull request Jul 21, 2021
18 tasks
@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
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

Successfully merging this pull request may close these issues.

2 participants