Call conversion functions directly #10

martins-mozeiko · 2018-08-20T23:29:38Z

This makes generated convert to and from function to call C functions directly instead of going through Python capsule API - that currently requires importing module, which can lead to a lot of imports for nested types in array. Now conversion functions are placed in separate libraries that link to each other. Python module libraries link to these new libraries.

See #9 for more details.

Sorry, I cannot provide any specific performance numbers right now, I can try to do that a bit later.

I have tested this change on Ubuntu 16 and Windows.

Connects to #9

mikaelarguedas · 2018-08-24T17:13:20Z

A run of CI on all platforms. It looks like the libraries of the dependencies cannot be found on MacOS.

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas · 2018-08-24T17:18:12Z

rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake

+    if(WIN32)
+      target_link_libraries(${_target_name_lib} "${_pkg_base}/Lib/${_pkg_filename}.lib")
+    else()
+      target_link_libraries(${_target_name_lib} "${_pkg_base}/lib/lib${_pkg_filename}.so")


On macos the generated libraries have the .dylib extension and not .so. This seems to be the reason it fails to build

mikaelarguedas

thanks @martins-mozeiko got the improvement!

The code looks good to me overall, there are 2 main things to fixup before rerunning CI:

library extension for macos
generate a single "*_python" library per package

More details below

mikaelarguedas · 2018-08-24T18:29:00Z

rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake

+  add_dependencies(
+    ${_target_name_lib}
+    ${rosidl_generate_interfaces_TARGET}${_target_suffix}
+    ${rosidl_generate_interfaces_TARGET}__rosidl_typesupport_c


Is the dependency on the typesupport_c lib needed for the new libraries?

Along the same lines, if we don't need to depend on specific typesupport, we may also be able to generate a single _python_ library instead of 1 per typesupport

Ok I gave that a shot at 7fbb832 and it seems to work fine with a single python library per package

mikaelarguedas · 2018-08-24T20:55:23Z

rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em

+#  define IMPORT_API __declspec(dllimport)
+#else
+#  define IMPORT_API
+#endif


As a header containing visibility macros in included on the next line (#include <rosidl_generator_c/visibility_control.h>), I'd recommend using the macros deinfed in that file instead of redefining new ones here.
The same logic could be applied to the other template below

mikaelarguedas

address review comments (used "comment" instead of "request changes" during my review :/)

dirk-thomas · 2018-09-06T17:18:45Z

@martins-mozeiko Is there any update in this?

This makes generated convert to and from function to call C functions direclty instead of going through Python capsule API. Conversion functions are placed in separate libraries that link to each other. Python module libraries links to these new libraries. See #9 for more details.

martins-mozeiko · 2018-09-07T01:39:31Z

I've added changes requested - now it uses dylib extension on macOS (I have not Mac available currently, so I cannot test it), and it uses visibility macros from visibility_control.h header instead of defining its own macros.

I've also integrated changes from Mikael and rebased all commits on top of master.

Tried building & running demo_nodes_py talker/listener - it works fine on Windows & Linux.

Code has been updated, needs new review

mikaelarguedas · 2018-09-10T19:27:23Z

Thanks @martins-mozeiko for iterating.

Tried building & running demo_nodes_py talker/listener - it works fine on Windows & Linux.

It looks like this change fails the linter tests on CI, you should be able to test the changes by running colcon test --packages-select <PACKAGES_TO_TEST>.

Linux
Linux-aarch64
macOS
Windows

I pushed some fixups:

spurious line change from my CMake code shuffling: 77009ae
remove duplication of visibility_control include: 617e0c6
remove new blank line in generated files to fix linter failures: f931dbd

With fixups (remainung rcl test failure expected and tracked at ros2/rmw_fastrtps#226):

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas · 2018-09-10T19:28:23Z

Sorry, I cannot provide any specific performance numbers right now, I can try to do that a bit later.

I didnt get a chance to test the performances improvements either. Do you happen to have more data in that matter?

mikaelarguedas · 2018-09-11T17:03:02Z

Change looks good to me 👍

@dirk-thomas do you want to provide a second review on this ? or should I merge as is ?

dirk-thomas

Beside the one comment about naming the new functions I think it would be good to reproduce the performance impact (since that is the whole reason for this patch).

rosidl_generator_py/resource/_msg_support.c.em

mikaelarguedas · 2018-09-12T23:47:25Z

think it would be good to reproduce the performance impact (since that is the whole reason for this patch).

I agree that having numbers regarding the performance improvement would be valuable. However, I think we agree that the new approach is cleaner / preferable to the current one regardless of the exact measured performance gain.

I think this function should use a double underscore between the package name and the field type in order to clearly separate the two from each other (same as between the field type and the fixed name).

I agree that would be are better naming pattern. Though this PR doesn't change the function names that are already implemented using this scheme. I am fine renaming them in here but would not be opposed in doing it in a follow-up as it's not strictly in the scope of this PR

…eld type

mikaelarguedas · 2018-09-13T18:15:24Z

Renamed the functions in 18d2af1

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas · 2018-09-13T18:28:02Z

@martins-mozeiko when you get a chance, can you please test the latest state of this PR and provide feedback on the (hopefully) time improvements compared to master?

martins-mozeiko · 2018-09-14T22:22:11Z

I'll try to do make some comparison. The issue is that difference is mostly visible only on weaker platforms like Pi. On powerful desktop I don't remember if I saw significant change (but I can recheck). And building for Pi using system we have here takes 4+ hours (arm qemu in docker). And I need to do two builds with and without patch. Also builds we have works only for ardent, I'll need to check how can I upgrade it to bouncy & colcon. So it'll take a while, but I'll look into it next week.

martins-mozeiko · 2018-09-17T22:19:49Z

I've made a following test - publisher sends geometry_msgs/PoseArray messages pretty much as fast as it can. And subscribe counts how long does it take to receive 100 messages.

Here is publisher.py code:

#!/usr/bin/env python3

import rclpy, rclpy.node
from geometry_msgs.msg import Pose, PoseArray

count = 0

def cb():
  global pub
  global count
  global msg

  pub.publish(msg)

  count += 1
  print("Sent", count)

msg = PoseArray()
msg.header.stamp.sec = 100
msg.header.stamp.nanosec = 200
msg.header.frame_id = "frame"
for i in range(1024):
  p = Pose()
  p.position.x = 1.0
  p.position.y = 2.0
  p.position.z = 3.0
  p.orientation.x = 4.0
  p.orientation.y = 5.0
  p.orientation.z = 6.0
  p.orientation.w = 7.0
  msg.poses.append(p)

rclpy.init()
n = rclpy.node.Node("publisher")
pub = n.create_publisher(PoseArray, "/test")
timer = n.create_timer(0.001, cb)

rclpy.spin(n)

And here is subscriber.py code:

#!/usr/bin/env python3

import rclpy, rclpy.node
from time import time
from geometry_msgs.msg import PoseArray

count = 0

def cb(msg):
  global count
  count += 1
  if count == 100:
    global start
    print(time() - start)
    exit()

rclpy.init()
n = rclpy.node.Node("listener")
sub = n.create_subscription(PoseArray, "/test", cb)

start = time()
rclpy.spin(n)

Running this on Raspberry Pi 3 with ROS2 built from master gives me ~63 seconds.

After applying this pull request the time decreases to ~37 seconds.

mikaelarguedas · 2018-09-18T16:11:45Z

Thanks @martins-mozeiko for testing and the example 👍

Tried a similar thing on amd64 with a few changes:

setting the start time when the first received message so that it's easier to reproduce
printing to console only once every 100th
stop after receiving a thousand message

Without this patch

$ PYTHONOPTIMIZE=0 python3 receiver.py 
16.82235884666443

With this patch:

$ PYTHONOPTIMIZE=0 python3 receiver.py 
14.596002578735352

Code

sender.py:

import rclpy, rclpy.node
from geometry_msgs.msg import Pose, PoseArray

count = 0

def cb():
    global pub
    global count
    global msg

    pub.publish(msg)

    count += 1
    if count % 100 == 0:
        print("Sent", count)

msg = PoseArray()
msg.header.stamp.sec = 100
msg.header.stamp.nanosec = 200
msg.header.frame_id = "frame"
for i in range(1024):
    p = Pose()
    p.position.x = 1.0
    p.position.y = 2.0
    p.position.z = 3.0
    p.orientation.x = 4.0
    p.orientation.y = 5.0
    p.orientation.z = 6.0
    p.orientation.w = 7.0
    msg.poses.append(p)

rclpy.init()
n = rclpy.create_node("publisher")
pub = n.create_publisher(PoseArray, "/test")
timer = n.create_timer(0.001, cb)

rclpy.spin(n)

receiver.py:

import rclpy
from time import time
from geometry_msgs.msg import PoseArray

count = 0
start = None


def cb(msg):
    global count
    global start
    if start is None:
        start = time()

    count += 1
    if count == 1000:
        global start
        print(time() - start)
        exit()


rclpy.init()
n = rclpy.create_node('listener')
sub = n.create_subscription(PoseArray, '/test', cb)

rclpy.spin(n)

Not as drastic as what is observed on the RaspberryPi but there is definitely an improvement

mikaelarguedas · 2018-09-18T22:29:21Z

Thanks @martins-mozeiko for the patch 👍

tfoote added the in review Waiting for review (Kanban column) label Aug 20, 2018

mikaelarguedas reviewed Aug 24, 2018

View reviewed changes

mikaelarguedas previously requested changes Aug 24, 2018

View reviewed changes

martins-mozeiko and others added 4 commits September 6, 2018 18:11

generate single _python lib per package

d57d938

use visibility macros from visibility_control.h

6125bc9

use dylib extension for libraries on macOS

7d8629c

mikaelarguedas added 3 commits September 10, 2018 10:46

spurious line change

77009ae

remove duplicated include

617e0c6

blank line fixups

f931dbd

mikaelarguedas mentioned this pull request Sep 11, 2018

Consider storing compiler flags in a single place and reuse them everywhere ros2/ros2#567

Open

mikaelarguedas added 4 commits September 11, 2018 09:06

revert unnecessary code relocation

5768b2e

rename variables and use lib folder instad of Lib on macos

c58d5da

create lowercase_field_type for readability and reuse

36010bc

remove debug prints

f8b7dbc

dirk-thomas reviewed Sep 11, 2018

View reviewed changes

rosidl_generator_py/resource/_msg_support.c.em Outdated Show resolved Hide resolved

rename functions to use double underscore between package name and fi…

18d2af1

…eld type

mikaelarguedas merged commit deab237 into ros2:master Sep 18, 2018

mikaelarguedas removed the in review Waiting for review (Kanban column) label Sep 18, 2018

This was referenced Sep 18, 2018

Improving conversion performance by avoiding repeated imports #9

Closed

use add_compile_options instead of setting only cxx flags ros2/common_interfaces#59

Merged

clalancette mentioned this pull request Jun 21, 2023

Install compiled libraries only to 'lib' #195

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call conversion functions directly #10

Call conversion functions directly #10

martins-mozeiko commented Aug 20, 2018 •

edited by mikaelarguedas

Loading

mikaelarguedas commented Aug 24, 2018

mikaelarguedas Aug 24, 2018

mikaelarguedas left a comment

mikaelarguedas Aug 24, 2018

mikaelarguedas Aug 24, 2018

mikaelarguedas Aug 24, 2018

mikaelarguedas Aug 24, 2018

mikaelarguedas left a comment •

edited

Loading

dirk-thomas commented Sep 6, 2018

martins-mozeiko commented Sep 7, 2018

mikaelarguedas commented Sep 10, 2018

mikaelarguedas commented Sep 10, 2018

mikaelarguedas commented Sep 11, 2018

dirk-thomas left a comment

mikaelarguedas commented Sep 12, 2018

mikaelarguedas commented Sep 13, 2018

mikaelarguedas commented Sep 13, 2018

martins-mozeiko commented Sep 14, 2018

martins-mozeiko commented Sep 17, 2018

mikaelarguedas commented Sep 18, 2018 •

edited

Loading

mikaelarguedas commented Sep 18, 2018

Call conversion functions directly #10

Call conversion functions directly #10

Conversation

martins-mozeiko commented Aug 20, 2018 • edited by mikaelarguedas Loading

mikaelarguedas commented Aug 24, 2018

mikaelarguedas Aug 24, 2018

Choose a reason for hiding this comment

mikaelarguedas left a comment

Choose a reason for hiding this comment

mikaelarguedas Aug 24, 2018

Choose a reason for hiding this comment

mikaelarguedas Aug 24, 2018

Choose a reason for hiding this comment

mikaelarguedas Aug 24, 2018

Choose a reason for hiding this comment

mikaelarguedas Aug 24, 2018

Choose a reason for hiding this comment

mikaelarguedas left a comment • edited Loading

Choose a reason for hiding this comment

dirk-thomas commented Sep 6, 2018

martins-mozeiko commented Sep 7, 2018

mikaelarguedas commented Sep 10, 2018

mikaelarguedas commented Sep 10, 2018

mikaelarguedas commented Sep 11, 2018

dirk-thomas left a comment

Choose a reason for hiding this comment

mikaelarguedas commented Sep 12, 2018

mikaelarguedas commented Sep 13, 2018

mikaelarguedas commented Sep 13, 2018

martins-mozeiko commented Sep 14, 2018

martins-mozeiko commented Sep 17, 2018

mikaelarguedas commented Sep 18, 2018 • edited Loading

mikaelarguedas commented Sep 18, 2018

martins-mozeiko commented Aug 20, 2018 •

edited by mikaelarguedas

Loading

mikaelarguedas left a comment •

edited

Loading

mikaelarguedas commented Sep 18, 2018 •

edited

Loading