search

ros2 / rosidl_python

rosidl support for Python
Apache License 2.0
20 stars 45 forks source link

Revamp how we check for the correct class. #218

Closed clalancette closed 1 month ago

clalancette commented 1 month ago

In an issue it was pointed out that we are decref'ing before we actually use some of the data, which means that in theory the garbage collector could reclaim the data before we used it. So the original point of this change was to fix that issue.

However, while looking at it I realized we could slightly improve performance here by avoiding a copy of the class and module into a combined string. Instead, we can compare them separately, which should reduce the copies.

This is a partial solution to https://github.com/ros2/rosidl_python/issues/217 ; the other reported bug in there will require a much more extensive rework.

clalancette commented 1 month ago

Pulls: https://github.com/ros2/rosidl_python/pull/218 Gist: https://gist.githubusercontent.com/clalancette/1319b32b184fce38189666d4e4e69c43/raw/2b4fb3b5d7b2c647a8f6fe22b9010efcd374125e/ros2.repos BUILD args: --packages-above-and-dependencies ament_cmake_core TEST args: --packages-above ament_cmake_core ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14647

clalancette commented 1 month ago

I had to add a fix when building in Release mode. Another set of CI is upcoming.

clalancette commented 1 month ago

Pulls: https://github.com/ros2/rosidl_python/pull/218 Gist: https://gist.githubusercontent.com/clalancette/1319b32b184fce38189666d4e4e69c43/raw/2b4fb3b5d7b2c647a8f6fe22b9010efcd374125e/ros2.repos BUILD args: TEST args: ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14647