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

Look at how to speed up the builds #17

Open
tfoote opened this issue Jan 16, 2024 · 20 comments
Open

Look at how to speed up the builds #17

tfoote opened this issue Jan 16, 2024 · 20 comments
Assignees

Comments

@tfoote
Copy link
Owner

tfoote commented Jan 16, 2024

There's a potential solution ros2/rosidl#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
Copy link
Owner Author

tfoote commented Jan 19, 2024

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
Copy link
Collaborator

gonzodepedro commented Jan 24, 2024

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.
Loading

@gonzodepedro
Copy link
Collaborator

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
Loading

@tfoote
Copy link
Owner Author

tfoote commented Jan 24, 2024

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
Copy link
Owner Author

tfoote commented Jan 26, 2024

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
Copy link
Collaborator

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
Copy link
Owner Author

tfoote commented Jan 29, 2024

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

@gonzodepedro
Copy link
Collaborator

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

@gonzodepedro
Copy link
Collaborator

gonzodepedro commented Feb 7, 2024

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
Loading

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
Loading

@gonzodepedro
Copy link
Collaborator

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

@tfoote
Copy link
Owner Author

tfoote commented Feb 7, 2024

  • Top level items would be great to have verbs with descriptions

    • aka Building a Package with message definitions", "invoking rosidl_generate_interfaces"
  • It would be nice to see what the outputs are "inputs => output file type/format"

  • Include the last stage that gets blown out in the feature on the diagram (add one more row/cycle showing the invocation of the generator in the top diagram)

@tfoote
Copy link
Owner Author

tfoote commented Feb 9, 2024

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
Copy link
Owner Author

tfoote commented Feb 23, 2024

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

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

@tfoote
Copy link
Owner Author

tfoote commented Feb 28, 2024

@gonzodepedro can you check out chris's concerns in ros2/rosidl#769 (comment)

@tfoote
Copy link
Owner Author

tfoote commented Mar 1, 2024

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

@gonzodepedro
Copy link
Collaborator

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

@gonzodepedro
Copy link
Collaborator

PR for rosidl "rebase" DanMesh/rosidl#1

@tfoote
Copy link
Owner Author

tfoote commented Mar 11, 2024

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

@tfoote
Copy link
Owner Author

tfoote commented Mar 15, 2024

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.

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

No branches or pull requests

2 participants