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

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

Open
wants to merge 81 commits into
base: master
Choose a base branch
from

Conversation

jakubDoka
Copy link
Contributor

@jakubDoka jakubDoka commented Jul 8, 2024

Description

I have rewritten NetworkBehavior derive macro to generate more optimal and faster to compile code when using more behaviours (5, 10, 20), I noticed performance degrades even though I benchmarked the same load. This is related to #5026.

New macro implementation generates enums and structs for each type implementing the traits instead of type-level linked lists. In many cases, this makes resulting types more compact (we store just one enum tag, whereas composed Eithers each need to store tags to make values addressable) and makes the enum dispatch constant. This also opened the opportunity to optimize UpgradeInfoIterator and ConnectionHandler into a state machine (they now remember where they stopped polling/iterating and skipped exhausted subhandlers/iterators). We could optimize the NetworkBehaviour itself too, but it would require users to put extra fields into the struct (this could be optional for BC).

Change checklist

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

note: I misused the sync feature on github and it erased all my changes from the branch, thus pr #5303 was closed @jxs, @thomaseizinger.

jakubDoka and others added 30 commits December 22, 2023 22:51
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
@jakubDoka
Copy link
Contributor Author

the compiler is now freaking out because the current macro is generating too nested types, this relates to the benchmark which uses swarm with up to 20 behaviors the command is:

cargo bench -p libp2p-swarm --features tokio,macros
error: reached the type-length limit while instantiating `<std::iter::Chain<std::iter::Map<std::iter::Chain<std::iter::Map<std::iter::Chain<..., ...>, ...>, ...>, ...>, ...> as Iterator>::fold::<..., ...>`
   --> /home/mlokis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/adapters/map.rs:129:9
    |
129 |         self.iter.fold(init, map_fold(self.f, g))
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider adding a `#![type_length_limit="184713361"]` attribute to your crate
    = note: the full type name has been written to '/home/mlokis/work/rust/rust-libp2p/target/release/deps/connection_handler-9e0d2f413cdbb0ba.long-type.txt'

when I use experimental macros:

cargo bench -p libp2p-swarm --features tokio,macros,experimental-macros

the compiler can deal with it fine

@jakubDoka
Copy link
Contributor Author

Should I switch all of the tests to the new macro?

@thomaseizinger
Copy link
Contributor

I think this is a great effort! I no longer maintain rust-libp2p but I am sure @jxs will be equally excited :)

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Hi @jakubDoka this is amazing work, I am sorry for only being able to review this now. LGTM only some minor remarks left

Should I switch all of the tests to the new macro?

can we repeat them to experimental as well?
Thanks!

swarm/bench.backup.txt Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm-derive/src/experimental.rs Outdated Show resolved Hide resolved
Copy link
Contributor

mergify bot commented Aug 5, 2024

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

@jakubDoka
Copy link
Contributor Author

@jxs it should be ready to merge.

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

hi @jakubDoka we don't need a version bump nor CHANGELOG.md entries for test changes, as it doesn't affect the published version

@jakubDoka
Copy link
Contributor Author

Well, the ci was breaking so that got me a bit confused, how do I work around that?

@jxs
Copy link
Member

jxs commented Aug 7, 2024

Well, the ci was breaking so that got me a bit confused, how do I work around that?

can you show what was breaking the CI? or rollback the CHANGELOG.md and Cargo.toml bumps so we can see

Copy link
Contributor

mergify bot commented Sep 25, 2024

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

@jakubDoka
Copy link
Contributor Author

@jxs Sorry for the wait, I did not do this the moment I saw the message so I forgot about it, you can see the changelog checks are breaking now

Copy link
Contributor

mergify bot commented Sep 26, 2024

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

Copy link
Contributor

mergify bot commented Oct 4, 2024

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

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.

4 participants