-
Notifications
You must be signed in to change notification settings - Fork 432
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
Refactor/eth66 #4758
Refactor/eth66 #4758
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, you are so fast!
I think we need benchmark to compare it before and after changes. Any ideas who can help us? Maybe @kamilchodola ?
src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V66/Eth66ProtocolHandler.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V66/Eth66ProtocolHandler.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it stack up comparing with previous version? Any improvements (doesn't have to be total time, but time in this area or memory)
src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V66/Eth66ProtocolHandler.cs
Show resolved
Hide resolved
src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V66/Eth66ProtocolHandler.cs
Outdated
Show resolved
Hide resolved
64f3a6c
to
2698b7d
Compare
The main question still is: Is this beneficial in any way, because it looks like it we already have request->response mapping through |
I think for now its largely for conformance reason. Maybe one day of the client would response out of order for performance reason, eg, some block is on cache some not. If that happen sync might break. |
Finally got some runs to complete. Before vs after (EPYC cpu):
In conclusion, I have terrible luck. None of the four pair have the same CPU. At least they all can sync. |
Another runs. Before vs after:
|
2a33da9
to
38ec20d
Compare
Resolves #4714
Changes:
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyTesting
Requires testing
In case you checked yes, did you write tests??
Comments about testing , should you have some (optional)
Further comments (optional)
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...