fix(swarm): eliminating protocol cloning when nothing is happening #5026

jakubDoka · 2023-12-22T21:57:17Z

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. Following are benchmark results:

behaviour count	iterations	protocols	timings	change*
1	1000	10	27.798 µs 28.134 µs 28.493 µs	-15.771% -14.523% -13.269%
1	1000	100	55.171 µs 55.578 µs 56.009 µs	-51.831% -50.162% -48.437%
1	1000	1000	289.24 µs 290.99 µs 293.00 µs	-61.748% -60.895% -60.054%
5	1000	2	34.000 µs 34.216 µs 34.457 µs	-18.538% -16.231% -14.011%
5	1000	20	70.962 µs 71.428 µs 72.005 µs	-40.501% -38.944% -37.309%
5	1000	200	426.17 µs 433.27 µs 442.60 µs	-44.824% -42.663% -40.262%
10	1000	1	42.993 µs 44.382 µs 45.655 µs	-18.839% -16.292% -13.584%
10	1000	10	94.022 µs 96.787 µs 99.321 µs	-25.469% -23.572% -21.562%
10	1000	100	543.13 µs 554.91 µs 569.06 µs	-43.781% -42.189% -40.568%
20	500	1	63.150 µs 64.846 µs 66.860 µs	-9.5693% -6.1722% -2.6400%
20	500	10	212.21 µs 217.48 µs 222.64 µs	-16.525% -14.234% -11.925%
20	500	100	1.6651 ms 1.7083 ms 1.7490 ms	-27.704% -25.683% -23.618%

change*: 3da7d91 is the baseline

Notes & open questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger

Great stuff! I have some questions. Also, did you manage to benchmark this somehow?

swarm/src/connection.rs

jakubDoka · 2023-12-25T06:55:37Z

Also, did you manage to benchmark this somehow?

I ll try few benchmark strategies and will see. This should improve, so long as protocols don't change with each poll.

jakubDoka · 2023-12-25T07:47:11Z

https://github.com/libp2p/rust-libp2p/pull/5026/files#diff-03e30a287d6b2160a5ec3615cbe96268d6a778f6c96656982d78946c3cb04dcbR935-R966

hashset (bacb93ccdbd3347052b063ca7252943297c2be50)
num protocols | time
2 564.58248ms
4 828.611434ms
10 1.632474501s
20 3.054404475s

vec (d8417ea274c8a7a15f4965bc3d6e18a5c7f27791)
num protocols | time
2 320.806934ms
4 420.014621ms
10 1.001984668s
20 2.789481624s

since we always insert all of the protocols to the hashset on each poll, it hinders the performance

jakubDoka · 2023-12-25T08:03:17Z

I am now using hashmap with booleans to compute the diff, so no need to collect the protocols.

hashmap (98b2eb1ca01ac0b02950d4871c68408e7093fa64)
num protocols | time
2 370.042196ms
4 497.035778ms
10 836.521122ms
20 1.435295081s

jakubDoka · 2023-12-25T08:07:50Z

finally this is results of benchmark on old code:

old code (b6bb02b9305b56ed2a4e2ff44b510fa84d8d7401)
num protocols | time
2 728.680186ms
4 1.292526676s
10 3.098013194s
20 6.180503327s

jakubDoka · 2023-12-25T08:09:13Z

@thomaseizinger I am curios what you think about the way I benchmark it

:

jakubDoka · 2023-12-25T09:13:52Z

I realized am testing with very short protocol names so here is a little change

old code (b6bb02b9305b56ed2a4e2ff44b510fa84d8d7401)
2 770.244421ms
4 1.382793447s
10 3.299081332s
20 6.912836208s

this pr (c271dbd76cb2013f1f976c2698be9d2c185e21f4)
2 402.820567ms
4 580.694491ms
10 956.659777ms
20 1.577939021s

swarm/src/connection.rs

thomaseizinger

Thank you for the updates! I've left some comments :)

I am liking the direction this is going in and looking forward to merge the performance improvements!

swarm/src/connection.rs

swarm/src/handler.rs

swarm/src/connection.rs

swarm/src/handler.rs

swarm/src/connection.rs

thomaseizinger

Thank you for the updates! I've left some more comments with questions and suggestions :)

swarm/src/handler.rs

swarm/src/connection.rs

jakubDoka · 2024-01-09T18:20:54Z

@thomaseizinger, hey, did I miss something that still needs to be done?

thomaseizinger · 2024-01-10T13:57:56Z

@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! :)

jakubDoka · 2024-06-05T23:32:29Z

that was the problem, scarry

thomaseizinger

This looks great! Thank you very much.

We have some automation that will use the text in ## Description in your PR as the commit message for the squash-merged commit.

Could you include some benchmark results in there? Just the criterion run is good with current master as the baseline?

jakubDoka · 2024-06-06T00:39:31Z

This looks great! Thank you very much.

We have some automation that will use the text in ## Description in your PR as the commit message for the squash-merged commit.

Could you include some benchmark results in there? Just the criterion run is good with current master as the baseline?

added

thomaseizinger · 2024-06-06T03:44:31Z

@jxs All yours from here.

jxs

Hi, and Sorry for the delay in the review. Looking forward to land this! Left just some minor comments.
Thanks

swarm/CHANGELOG.md

swarm/src/lib.rs

swarm/benches/connection_handler.rs

swarm/src/connection.rs

mergify · 2024-06-14T20:35:28Z

This pull request has merge conflicts. Could you please resolve them @jakubDoka? 🙏

Co-authored-by: João Oliveira <[email protected]>

jxs

Sorry for the delay. LGTM! Thanks @jakubDoka!

swarm/src/connection.rs

Approvals have been dismissed because the PR was updated after the send-it label was applied.

swarm/src/connection.rs

swarm/benches/connection_handler.rs

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. Following are benchmark results: |behaviour count|iterations|protocols|timings|change*| |-|-|-|-|-| |1|1000|10|27.798 µs 28.134 µs 28.493 µs|-15.771% -14.523% -13.269%| |1|1000|100|55.171 µs 55.578 µs 56.009 µs|-51.831% -50.162% -48.437%| |1|1000|1000|289.24 µs 290.99 µs 293.00 µs|-61.748% -60.895% -60.054%| |5|1000|2|34.000 µs 34.216 µs 34.457 µs|-18.538% -16.231% -14.011%| |5|1000|20|70.962 µs 71.428 µs 72.005 µs|-40.501% -38.944% -37.309%| |5|1000|200|426.17 µs 433.27 µs 442.60 µs|-44.824% -42.663% -40.262%| |10|1000|1|42.993 µs 44.382 µs 45.655 µs|-18.839% -16.292% -13.584%| |10|1000|10|94.022 µs 96.787 µs 99.321 µs|-25.469% -23.572% -21.562%| |10|1000|100|543.13 µs 554.91 µs 569.06 µs|-43.781% -42.189% -40.568%| |20|500|1|63.150 µs 64.846 µs 66.860 µs|-9.5693% -6.1722% -2.6400%| |20|500|10|212.21 µs 217.48 µs 222.64 µs|-16.525% -14.234% -11.925%| |20|500|100|1.6651 ms 1.7083 ms 1.7490 ms|-27.704% -25.683% -23.618%| change*: 3da7d91 is the baseline Pull-Request: libp2p#5026.

eliminating protocol cloning when nothing is happening

40ff8d1

jakubDoka changed the title ~~eliminating protocol cloning when nothing is happening~~ fix(swarm): eliminating protocol cloning when nothing is happening Dec 22, 2023

jakubDoka mentioned this pull request Dec 22, 2023

fix(swarm): Eliminating repetitive arc construction on each poll #4991

Closed

4 tasks

thomaseizinger reviewed Dec 23, 2023

View reviewed changes

swarm/src/connection.rs Outdated Show resolved Hide resolved

swarm/src/connection.rs Outdated Show resolved Hide resolved

swarm/src/connection.rs Outdated Show resolved Hide resolved

jakubDoka and others added 2 commits December 25, 2023 06:46

Merge branch 'master' into master

d8417ea

switching to hashsets

b9ffdae

jakubDoka added 2 commits December 25, 2023 08:20

adding benchmark

6628dd4

modifiing benchmark

bacb93c

optimizing further

98b2eb1

making the protocol name lengths more realiztic

c271dbd

:

jakubDoka force-pushed the master branch from a297012 to c271dbd Compare December 25, 2023 09:13

Merge branch 'master' into master

d5edce0

thomaseizinger reviewed Dec 27, 2023

View reviewed changes

swarm/src/connection.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Dec 27, 2023

View reviewed changes

jakubDoka and others added 3 commits December 28, 2023 13:47

Merge branch 'master' into master

b7ab499

reducing dif

5860200

adding benchmark

adaaf7a

thomaseizinger reviewed Dec 29, 2023

View reviewed changes

jakubDoka and others added 4 commits December 29, 2023 16:11

appliing suggestions

016a3fa

Merge branch 'master' into master

a7ec976

Merge branch 'master' into master

3a53071

Merge branch 'master' into master

8ff2371

reverting cargo lock

d587578

jakubDoka and others added 2 commits June 6, 2024 01:44

I do hate this

1b9fc1f

Merge branch 'master' into master

9be7e50

thomaseizinger previously approved these changes Jun 5, 2024

View reviewed changes

jxs reviewed Jun 14, 2024

View reviewed changes

swarm/CHANGELOG.md Outdated Show resolved Hide resolved

swarm/src/lib.rs Show resolved Hide resolved

swarm/benches/connection_handler.rs Show resolved Hide resolved

swarm/src/connection.rs Show resolved Hide resolved

jakubDoka and others added 5 commits June 27, 2024 11:42

Update swarm/CHANGELOG.md

42cdc6a

Co-authored-by: João Oliveira <[email protected]>

Update swarm/src/lib.rs

a7a55c1

Co-authored-by: João Oliveira <[email protected]>

Merge branch 'master' into master

2d75d6d

adding some clarification about AsAtrHashEq

71ff5f1

ups

75eed8c

jxs added this to the v0.54.0 milestone Jul 5, 2024

Merge branch 'master' into master

7efc173

jxs previously approved these changes Jul 5, 2024

View reviewed changes

swarm/src/connection.rs Outdated Show resolved Hide resolved

Update swarm/src/connection.rs

4955df6

jxs added the send-it label Jul 5, 2024

jxs reviewed Jul 5, 2024

View reviewed changes

swarm/src/connection.rs Outdated Show resolved Hide resolved

Update swarm/src/connection.rs

a15340e

jxs reviewed Jul 5, 2024

View reviewed changes

swarm/benches/connection_handler.rs Outdated Show resolved Hide resolved

swarm/benches/connection_handler.rs Show resolved Hide resolved

jxs added 2 commits July 6, 2024 00:18

Update swarm/benches/connection_handler.rs

7d1a9d7

Update swarm/benches/connection_handler.rs

393149a

jxs approved these changes Jul 5, 2024

View reviewed changes

mergify bot merged commit 02fc027 into libp2p:master Jul 5, 2024
72 checks passed

jakubDoka mentioned this pull request Jul 8, 2024

fix(swarm): rewrite NetworkBehaviour macro for more optimal code gen #5486

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(swarm): eliminating protocol cloning when nothing is happening #5026

fix(swarm): eliminating protocol cloning when nothing is happening #5026

jakubDoka commented Dec 22, 2023 •

edited

Loading

thomaseizinger left a comment

jakubDoka commented Dec 25, 2023

jakubDoka commented Dec 25, 2023

jakubDoka commented Dec 25, 2023

jakubDoka commented Dec 25, 2023

jakubDoka commented Dec 25, 2023

jakubDoka commented Dec 25, 2023

thomaseizinger left a comment

thomaseizinger left a comment

jakubDoka commented Jan 9, 2024

thomaseizinger commented Jan 10, 2024

jakubDoka commented Jun 5, 2024 •

edited

Loading

thomaseizinger left a comment

jakubDoka commented Jun 6, 2024 •

edited

Loading

thomaseizinger commented Jun 6, 2024

jxs left a comment

mergify bot commented Jun 14, 2024

jxs left a comment

fix(swarm): eliminating protocol cloning when nothing is happening #5026

fix(swarm): eliminating protocol cloning when nothing is happening #5026

Conversation

jakubDoka commented Dec 22, 2023 • edited Loading

Description

Notes & open questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

jakubDoka commented Dec 25, 2023

jakubDoka commented Dec 25, 2023

jakubDoka commented Dec 25, 2023

jakubDoka commented Dec 25, 2023

jakubDoka commented Dec 25, 2023

jakubDoka commented Dec 25, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

jakubDoka commented Jan 9, 2024

thomaseizinger commented Jan 10, 2024

jakubDoka commented Jun 5, 2024 • edited Loading

thomaseizinger left a comment

Choose a reason for hiding this comment

jakubDoka commented Jun 6, 2024 • edited Loading

thomaseizinger commented Jun 6, 2024

jxs left a comment

Choose a reason for hiding this comment

mergify bot commented Jun 14, 2024

jxs left a comment

Choose a reason for hiding this comment

jakubDoka commented Dec 22, 2023 •

edited

Loading

jakubDoka commented Jun 5, 2024 •

edited

Loading

jakubDoka commented Jun 6, 2024 •

edited

Loading