-
-
Notifications
You must be signed in to change notification settings - Fork 343
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
feat: move libp2p to network worker thread #5229
Conversation
why should this happen? |
@wemeetagain Can a worker spawn another worker? |
Afaik yes |
cee8592
to
74fd057
Compare
@wemeetagain regarding ReqResp
|
@dapplion I reviewed the changes majorly focused to the The points I can't understand is why this effort is been done with very particular focus on the
We may definitely start with network, but then the focus for foreseeable future will be clear. e.g. I foresee the similar arguments for the Rest interface as well, but can't align how this refactor would enable us to do that next. |
The network is the source of all untrusted and adversarial input so it holds the vast amount of the risk. So the network is a natural candidate to split out first, with medium cost and high benefit. We can offload a lot of normal work and worst-case work from the main thread, and the network somewhat decoupled from need for fast access to our chain. All connection management (esp in DoS cases), network crypto (handshakes, connection encryption/decryption), simple req/resp (ping, status, metadata) can be handled entirely off-main-thread. My preference for further splitting is that we are careful to avoid splitting 'just because', and have a clear sense of the benefits. |
3d76f24
to
8cdd0c5
Compare
Rebased branch on current unstable with almost everything implemented and wired. Pushed previous branch into backup branch to review if necessary https://github.com/ChainSafe/lodestar/compare/dapplion/network-thread-may1 CC @wemeetagain |
8cdd0c5
to
6ba3c21
Compare
6ba3c21
to
516b44e
Compare
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
Big remaining item is the logger in the thread
CC @wemeetagain |
ddef290
to
b16d9e1
Compare
Attached is the profile of network thread from lg1k node I was not able to do that by Chrome Dev Tool but with VsCode |
* feat: add ThreadBoundaryError * Remove ClonableLodestarError * Fix unit test --------- Co-authored-by: Cayman <caymannava@gmail.com>
🎉 thanks @wemeetagain ❤️ |
🎉 This PR is included in v1.9.0 🎉 |
Motivation
Goals
Description
TBD
TODO
Move discv5 worker out of libp2p worker, and connect them via main threadCloses #5447