Using C extention base class for every message

[almaslov]

Added C extension base class for every message. These base classes define native fields and corresponding slots(aka struct PyMemberDef). During deserialization they allow to call __new__(which is C function) instead of expensive __init__(which is pure Python) and initialize base class C fields instead of Python class members.

Details are in https://github.com/ros2/rosidl_python/issues/192

[almaslov]

As far as I can understand errors in CI, my code depends on commit from rosidl_parser that is not tagged yet, and so not available in latest package from http://repo.ros2.org/ubuntu/testing. So CI will fill for all branches forked from current rolling.

[clalancette]

So CI will fill for all branches forked from current rolling.

Yeah, that's correct. We need to do some releases, hopefully later this week. We have other CI that we can run (by hand), so after this goes through review we can rely on that.

[Voldivh]

Hi @almaslov, I was trying to test out your solution. However, I encountered an error during compilation once I switched to your branch. The error was:

make[2]: *** No rule to make target 'rosidl_generator_py/rosidl_generator_py/_rosidl_generator_py_bases.c', needed by 'CMakeFiles/rosidl_generator_py_custom__bases.dir/rosidl_generator_py/rosidl_generator_py/_rosidl_generator_py_bases.c.o'.  Stop.

I believe this line is not generating the appropriate files or is not generating them on the proper path. Could you take a look at it?

[almaslov]

Hello! Thank you for your time! I've just tried to build rosidl_generator_py from scratch in fresh ubuntu:jammy docker container and had no problems. I've walked through this guide to setup environment for my build: https://docs.ros.org/en/rolling/Installation/Alternatives/Ubuntu-Development-Setup.html .

_bases.c file is generated in build/rosidl_generator_py/rosidl_generator_py/rosidl_generator_py/_rosidl_generator_py_bases.c as expected. I believe I had this issue while development and cleaning package build directory fixed it. If it doesn't help in your case, could you please describe your environment.

[Voldivh]

Hi! After cleaning the environment it compiled without issue, I did notice that the problem happens when you try to compile on top of a previous build from other branches.

The test I was trying to do was to publish a message (in particular a Image from the sensor_msgs) and measure the time it took from publishing the message until the subscriber received it. I believe that the message should pass through the serialization and deserealization of the message. I tried out that test with the rolling branch of this repository and the branch from your PR as well and both yielded basically the same amount of time. I thought I was going to be able to notice a difference in the latency of the messages when switching the branches. Could you perhaps point me in the right direction to test out your branch improvement? I'm not sure if perhaps the message was too small (The image was something around 200Kb) as to notice a difference in performance or if I should go with a custom message with several nested data structures. The way I was measuring the time was using the clock from the node, store that value in the header timestamp and checking the time again on the subscriber's callback.

[almaslov]

As I can't see from same experiment on my local machine, deserialization takes about 150 microseconds and message transferring - 1150 microseconds. Deserialization on my feature branch takes about 130 microseconds, so optimization gain is less than 2%. I think the reason is that most of time is spent in memcpy'ing 200kb from C array to numpy array. I believe here is an area for optimization too, but it is out of current changes scope.

or if I should go with a custom message with several nested data structures.

I think this case will be more demonstrative. For example array of geographic_msgs/GeoPointStamped(as in my project) or TestArrayComplex from this issue

[Voldivh]

I think this case will be more demonstrative. For example array of geographic_msgs/GeoPointStamped(as in my project) or TestArrayComplex from this issue

I like this idea, I'll go ahead and test it out with those messages and post the results here. As for the PR, it seems to be compiling without any issues and is passing all the test on on my docker container with ubuntu::focal, I'll try it out on ubuntu::jammy as well but I don't think I would get any difference. We should wait for the next release and start another CI after that.

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-02-16/29927/1

[Voldivh]

Hi @almaslov, sorry for the late response. I tested out the PR taking into account the message types and ubuntu distros as we talked before and I did notice an improvement as the data structures were more complex and had nested structures, as we expected it to happen. As for the building, everything went fine without any issues during compilation and testing.

Friendly ping to @clalancette , is the next rolling release out? I believe we can trigger a new CI in order to confirm everything is fine with merging this with upstream.

[clalancette]

Friendly ping to @clalancette , is the next rolling release out? I believe we can trigger a new CI in order to confirm everything is fine with merging this with upstream.

Yes, it is out. I'll run the Rpr job again, but note that we'll also want to review this code and run full CI on it.

[clalancette]

@ros-pull-request-builder retest this please

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-03-16/30432/1

[almaslov]

I’ve made some changes to make deinitialization for imported modules in assumption, that subinterpreters support is not expected yet. There is now a single function @(package_name)__lazy_import which accepts integer parameter - index of module to be imported. Implementation is in @(package_name)_import.c souce file. Also I’ve added examples of generated files to example directory for review purpose. I will delete it before merging.

[EsipovPA]

Hello everyone!

I've took the liberty of updating this PR. I've notices that there were some conflicts with the rolling branch. Looks like these are done with.

Is it OK to request a new review? @almaslov @Voldivh