fix(swarm): eliminating protocol cloning when nothing is happening #5026
+953
−88
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakubDoka
changed the title
4 tasks
I ll try few benchmark strategies and will see. This should improve, so long as protocols don't change with each poll. |
since we always insert all of the protocols to the hashset on each poll, it hinders the performance |
|
I am now using hashmap with booleans to compute the diff, so no need to collect the protocols. |
|
finally this is results of benchmark on old code: |
|
@thomaseizinger I am curios what you think about the way I benchmark it |
|
I realized am testing with very short protocol names so here is a little change |
|
@thomaseizinger, hey, did I miss something that still needs to be done? |
Sorry for the delay. I am on low availability until mid-Jan. Will give this a review after! :) |
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
jxs
reviewed
Apr 5, 2024
| @@ -1 +1,2 @@ | |||
| target | |||
| perf.* | |||
There was a problem hiding this comment.
this is not required for the PR right?
|
FYI the new kid on the block of benchmarking doesn't need re-exports: https://nikolaivazquez.com/blog/divan/ |
|
Hey, @thomaseizinger, sorry for the delay, I was putting this off for a bit too long, here are my findings with criterion: This is the result of reverting optimizations with 10000 protocols on one behavior (compared to the run with changes in this PR), In this case, a lot more code is being executed than just the connection handler which might be the reason the difference is smaller. Please review the benchmarking code, I am not 100% confident this is a good measurement. I'll also try making Tokio run in single-threaded mode if that makes a difference. |
|
does memory transport deadlock on single-threaded mode? |
|
welp, I can't find any reasonable difference now, I guess the protocol drops are not that significant when all the other code is run as well, so I was most likely measuring with perf incorrectly |
|
Okay, @thomaseizinger, so, I had a bug in my benchmark, where it did the computation only in the first iteration, that explains why nothing made sense. Here are the results relative to the optimized version with code actually running: The scaling is crazy, I am also not sure if I still have bug in there. I will make the many behaviours few protocols too. |
|
Here are the results I measured when backporting the benchmark to the old code (relative to the code in this PR).
I am still suspicious of some of the gigantic performance differences, but this may be due to the optimized version avoiding protocol cloning. Full Results |
|
Benchmark results with applied modifications. Numbers are now more reasonable since both
|
4 tasks
There was a problem hiding this comment.
Thanks! These numbers make a bit more sense. I have two more comments that could simplify the benchmarking code a bit. I'd like to make sure we have as little complexity in there as possible. The restarting state irks me a bit, I'd rather defer any form of setup to criterion to make sure we aren't messing up any setup of the benchmark.
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
|
I am currently travelling but will look at this in 2ish weeks time. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Code keeps the API while eliminating repetitive protocol cloning when protocols did not change, If protocol changes occur, only then the protocols are cloned to a reused buffer from which they are borrowed for iteration.
Notes & open questions
Change checklist