tfoote / rosidl_typesupport_protobuf

Middleware agnostic ros2 static typesupport which uses Protobuf for serialization/deserialization.
Apache License 2.0
0 stars 0 forks source link

Look at how to speed up the builds #17

Open tfoote opened 10 months ago

tfoote commented 10 months ago

There's a potential solution https://github.com/ros2/rosidl/pull/769 for speeding up the build.

That might be a good solution or some other one such that the message generation phase goes faster and isn't noticable when building.

tfoote commented 10 months ago

Lets start with making a good diagram of how things work, and how they are extended to generate protobuf generation. Then make a parallel diagram of how the new system works and how it would be extended for generating protobuf.

Keeping in mind that there might be new generators of interest such as a rust generator in the future.

gonzodepedro commented 10 months ago

CURRENT Hook registration on CodeGenerator project build

sequenceDiagram
    participant CodeGenerator_cmake
    participant ament_package
    participant ament_register_extension
    CodeGenerator_cmake->>ament_package: CONFIG_EXTRAS
    ament_package->>ament_register_extension: rosidl_generate_idl_interfaces, cmake_code_hook
    Note right of ament_package: Execute a cmake.in template with variables and a cmake code hook.
gonzodepedro commented 10 months ago

CURRENT Hook execution and code generation

sequenceDiagram
    participant project_with_rosidl_files
    participant rosidl_generate_interfaces
    participant rosidl_generate_idl_interfaces_HOOK
    participant generate_arguments_file
    participant rosidl_generator
    project_with_rosidl_files->>rosidl_generate_interfaces: path_to_idl_files
    loop ForEach Hook
        rosidl_generate_interfaces->>rosidl_generate_idl_interfaces_HOOK: path_to_idl_files, parameters
        rosidl_generate_idl_interfaces_HOOK->>generate_arguments_file: template_parameters
        activate generate_arguments_file
        generate_arguments_file->>rosidl_generate_idl_interfaces_HOOK: arguments_file
        deactivate generate_arguments_file
        rosidl_generate_idl_interfaces_HOOK->>rosidl_generator: arguments_file
    end
tfoote commented 10 months ago

This will be great to add to documentation either here: https://github.com/ros2/rosidl?tab=readme-ov-file

Or where it links to at https://docs.ros.org/en/rolling/Concepts/Advanced/About-Internal-Interfaces.html

tfoote commented 10 months ago

Drafted a PR for ros2_documentation for old and new approach. Initial testing of new approach going well.

Do some initial benchmarking to validate speedup and that it works. Only some of the implementation, only rosidl_introspection_c and introspection_cpp are ported at the moment. The others rely on the backwards compatibility. We can show speedup for these and open tickets to accelerate the other groups.

We'll work to land the new method and then port the protobuf version to the new format. Showing speedup here will be a good case for others to use the new generator.

gonzodepedro commented 10 months ago

Documentation PRs created at my local fork for review:

Issues Encountered

  1. Not sure if mermaid is support on .rsl files
  2. Commited the mermaid sources and also a locally generated svg file.
tfoote commented 10 months ago

For Mermaid support we should try to use the sphinx plugin: https://github.com/mgaitan/sphinxcontrib-mermaid

gonzodepedro commented 10 months ago

I made changes to both PRs to reflect comments and work done by Tully to integrate mermaid into sphinx.

gonzodepedro commented 9 months ago

Currently, the sequence of events to generate idl interfaces looks something like this:

    sequenceDiagram
    participant interfaces project
    participant invoking rosidl_generate_interfaces
    participant invoking rosidl_generate_idl_interfaces HOOK
    participant invoking generate_arguments_file
    participant invoking rosidl_generator
    participant python interpreter
    participant em interpreter
    interfaces project->>invoking rosidl_generate_interfaces: path_to_idl_files
    loop ForEach Hook
        invoking rosidl_generate_interfaces->>invoking rosidl_generate_idl_interfaces HOOK: path_to_idl_files, parameters
        invoking rosidl_generate_idl_interfaces HOOK->>invoking generate_arguments_file: template_parameters
        activate invoking generate_arguments_file
        invoking generate_arguments_file->>invoking rosidl_generate_idl_interfaces HOOK: arguments_file
        deactivate invoking generate_arguments_file
        invoking rosidl_generate_idl_interfaces HOOK->>invoking rosidl_generator: arguments_file
        invoking rosidl_generator->>python interpreter: arguments_file
        Note right of invoking rosidl_generator: Kickstarting Python interpreter is an expensive task
        python interpreter->>em interpreter: template, arguments
    end

As you can see in the diagram above, calling rosidl_generator which generates code from templates and parameters, requires to kick-start a python interpreter, which is an expensive operation in terms of time and resources, this expensive operation happens inside a loop. Which means that the expensive operation will be executed once per interfaces per package.

This PR, allows to defer the call to the python interpreter for a later time, where all the interfaces and packages have already been executed. To do that, each interface package will, instead of calling the rosidl_generator directly, they will generate an arguments file and return it. When all interface packages are called and all arguments files are generated, then the code will call the rosidl_generator once, passing all the arguments_files.

As you will see in the diagram below, the original loop is now separated into three loops, the first one gets everything set, the second one generates the arguments files and the last one does all the post processing after the source code is generated. But the interpreter is instantiated only once, outside of any loop.

    sequenceDiagram
    participant interfaces project
    participant invoke rosidl_generate_interfaces
    participant invoke rosidl_create_type_descriptions_extensions HOOK
    participant invoke rosidl_generate_idl_interfaces HOOK
    participant invoke rosidl_write_generator_arguments_extensions HOOK
    participant invoke generate arguments_file
    participant invoke rosidl_generator
    participant python interpreter
    participant em interpretar
    interfaces project->>invoke rosidl_generate_interfaces: path_to_idl_files
    loop ForEach rosidl_create_type_descriptions_extensions Hook
        invoke rosidl_generate_interfaces->>invoke rosidl_create_type_descriptions_extensions HOOK: path_to_idl_files, parameters
    end
    loop ForEach rosidl_write_generator_arguments_extensions Hook
        invoke rosidl_generate_interfaces->>invoke rosidl_write_generator_arguments_extensions HOOK: path_to_idl_files, parameters
        activate invoke generate_arguments_file
        invoke rosidl_write_generator_arguments_extensions HOOK->>invoke generate_arguments_file: path_to_idl_files, parameters
        invoke generate_arguments_file->>invoke rosidl_write_generator_arguments_extensions HOOK: arguments_file
        deactivate invoke generate_arguments_file
        invoke rosidl_write_generator_arguments_extensions HOOK->>invoke rosidl_generate_interfaces: arguments_file
    end
    invoke rosidl_generate_interfaces->>invoke rosidl_generator: [arguments_files]
    invoke rosidl_generator->>python interpreter: arguments_file
    Note right of invoking rosidl_generator: Kickstarting Python interpreter is an expensive task
    python interpreter->>em interpreter: template, arguments
    loop ForEach invoke rosidl_generate_idl_interfaces Hook
        invoke rosidl_generate_interfaces->>invoke rosidl_generate_idl_interfaces HOOK: generated_files
    end
gonzodepedro commented 9 months ago

@tfoote PTAL at comment above, as a candidate for the description of the PR

tfoote commented 9 months ago
tfoote commented 9 months ago

Reworking this to be same language as the upstream PR to be contributed there.

Goal for next week extended documentation and benchmark showing upstream benefits to promote that PR out of draft status.

tfoote commented 9 months ago

It would be good to benchmark the processes. I found some resources such as:

https://stackoverflow.com/a/67012173/604099

tfoote commented 9 months ago

For reference other generators

rosidl_generator_java

https://github.com/ros2-java/ros2_java/blob/main/rosidl_generator_java/rosidl_generator_java/__init__.py

rclrs

https://github.com/ros2-rust/ros2_rust/blob/main/rosidl_generator_rs/rosidl_generator_rs/__init__.py

rosjava did catchup generation https://github.com/rosjava/rosjava_bootstrap/tree/kinetic/message_generation/src/main/java/org/ros/message

tfoote commented 9 months ago

@gonzodepedro can you check out chris's concerns in https://github.com/ros2/rosidl/pull/769#issuecomment-1738050659

tfoote commented 9 months ago

Top priorities rebase the rosidl change to be able to retest it. Work on documentation before landing.

gonzodepedro commented 9 months ago

Mermaid documentation https://mermaid.js.org/syntax/sequenceDiagram.html

gonzodepedro commented 8 months ago

PR for rosidl "rebase" https://github.com/DanMesh/rosidl/pull/1

tfoote commented 8 months ago

Also validating it with drake-ros to make sure it's working with non cmake projects

tfoote commented 8 months ago

The drake_ros work isn't using the command line interface, but introspecting/loading the

Instead make a python test in the typesupport libraries. Do this for protobuf_tyepsupport

And in the type adapter libraries and then our adapters

If the pattern is easy enough to replicate we can PR it for other typesupports in the future.