Skip to content

Refactor code generation using quote, syn and rayon. #58

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

Closed

Conversation

jerry73204
Copy link
Contributor

@jerry73204 jerry73204 commented Jun 22, 2023

This work thoroughly rewrites the code generation parts of this project. With the help of syn and quote, it processes code in tokens instead of string manipulation. It applies several tricks to speed up the codegen process, including parallelism and bindgen allowlist. It fixes the doc-only feature so that it can create API docs without ROS installed.

  • It uses quote and syn crates to parse the bindgen code into tokens instead of string manipulation. The approach promises error-free code generation. It fixes a known bug that it did not parse array typed fields [T; N] correctly.
  • This work uses rayon to speed up the code generation process. It enables build.rs to process many RosMsg in parallel on a multi-core machine, and now the build.rs can finish in a few seconds.
  • This work uses compile-time generated phf::Map to speed up the lookup process in runtime. The changes are made in UntypedServiceSupport::new_from(), INTROSPECTION_FNS, and CONSTANTS_MAP.
  • A list of allowed functions and constants is passed to bindgen to speed up the codegen process. Though it's still the slowest part overall, it still saves a few minutes.
  • It rewrites extern "C" functions in bindgen-generated code to empty functions. It enables doc-only feature to produce API docs without actually linking ROS dynamic link libraries.

@jerry73204
Copy link
Contributor Author

The docs_no_ros CI failed, but the log shows no explicit compile error. It just failed by a SIGKILL. I suspect it included too many ROS packages (100+ ROS packages on my PC) and caused the memory to explode.

@jerry73204 jerry73204 mentioned this pull request Jun 24, 2023
6 tasks
@m-dahl
Copy link
Collaborator

m-dahl commented Jun 25, 2023

Great work @jerry73204 . As discussed in #50 lets go ahead with this pr. I have just rebased this on top of #51 and pushed to master, so it will be easier to make new PRs from here in case @JiangengDong wants to contribute.

I also fixed up the last commit with a refresh of the the docs with just the standard ros install to make it pass CI.

@m-dahl m-dahl closed this Jun 25, 2023
@jerry73204
Copy link
Contributor Author

jerry73204 commented Jun 25, 2023

@m-dahl Thanks you merge my work. Just a reminder that the generated bindings in this PR are huge. That's because I installed almost everything on my laptop. It will cause a very long compile time (~5 minutes) for the doc-only feature.

Could you please re-generate the bindings for docs.rs using this IDL_PACKAGE_FILTER? The package list comes from the official ROS 2 doc here.

export IDL_PACKAGE_FILTER='action_msgs;diagnostic_msgs;geometry_msgs;lifecycle_msgs;map_msgs;move_base_msgs;nav_msgs;pendulum_msgs;rosgraph_msgs;sensor_msgs;shape_msgs;statistics_msgs;std_msgs;stereo_msgs;test_msgs;tf2_geometry_msgs;tf2_msgs;trajectory_msgs;unique_identifier_msgs;visualization_msgs'

I use this procedure to refresh the bindings.

rm -rf target
rm r2r*/bindings/*
cargo build --features save-bindgen
cargo build --features doc-only  # make sure the bindings work

jerry73204 added a commit to jerry73204/r2r that referenced this pull request Jun 25, 2023
@m-dahl
Copy link
Collaborator

m-dahl commented Jun 25, 2023

Yes I noticed it was huge so I modified your last commit before pushing. I refreshed them using humble-desktop + test_msgs.

Perhaps we should make a script for refreshing using that standard package list?

m-dahl added a commit that referenced this pull request Jun 26, 2023
Refresh bindings with new codegen by #58
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.

2 participants