-
Notifications
You must be signed in to change notification settings - Fork 46
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
Hides the assertions that checks the data types of the message fields. #194
Conversation
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general idea looks good to me. I've left a few things that I think should be fixed. Once that is done, I think we can run CI on it.
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more things to fix for flake8.
…le to avoid duplications Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
So after running some of the failed tests locally, it looks like there is another problem here. For instance, if you run the tests for
It looks like somehow |
Hmm Interesting, I'll take a look and see what I can do to solve it. |
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
After a little research and an internal discussion. We found out that the reason why some packages such as |
So with the fixes elsewhere, I think we are getting closer here. However, I think we have another problem in this repository. In particular, we want the rosidl_python/rosidl_generator_py/resource/_msg.py.em Lines 362 to 386 in e6d7248
What I'll suggest here is to just switch that
_ when we were using slots.
|
I see your point. I'll get right down to it. However, I think that we might actually encounter similar problems in other packages, is it worth that I do a little search and modifications or should we just leave it as it is as long as CI passes? @clalancette |
It is worthwhile doing a search through the existing ROS 2 core and see if there are other uses of |
…method instead of __slots__ Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
…epr__ method Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
…m the SLOT_TYPES attribute Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the comment inline, we still have to fix the __repr__
method in here to generate the correct output (using the zip
method again).
@@ -364,7 +370,7 @@ if isinstance(type_, AbstractNestedType): | |||
typename.pop() | |||
typename.append(self.__class__.__name__) | |||
args = [] | |||
for s, t in zip(self.__slots__, self.SLOT_TYPES): | |||
for s, t in self.get_fields_and_field_types().items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be fixed to use zip(self.get_fields_and_field_types().keys, self.SLOT_TYPES)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you are right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…and SLOT_TYPES Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
So I did a bit more digging into how To do this, I fetched all of the code that is currently released into rolling with the following:
(this takes a while). I then grepped through all of the code to find uses of
There are a total of 122 uses of However, I did find a number of uses of
I think we should attempt to fix these before we put this in, otherwise things will start failing and it will be hard to tell why. @Voldivh can you start taking a look at this, particularly |
Yeah, after looking a little bit to the packages you mentioned, I can see that they will require some fixing. I'll start to work on the ones you commented. |
Just finished creating the PRs that solve the issues with the packages: |
OK, so we now have the 4 in-core uses of slots fixed. I think it is worthwhile to open new issues or PRs for |
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
577cfa5
to
3dbb584
Compare
@Voldivh OK, this is looking good to me with CI. Can you please open up a PR to https://github.com/ros2/ros2_documentation/blob/rolling/source/Releases/Release-Iron-Irwini.rst , saying that we changed this behavior? Once that is ready, I'll merge that and this one together. |
Context
This PR is related to the issue described in this discussion. After doing some benchmarks on simple publishers and subscribers on rclpy using different types of messages (Ex: Strings, Ints, Arrays, Custom messages with nested structures) and analyzing their behavior using flamegraphs, we noticed that a lot of the busy time of the nodes was being used during the creation of the messages object. Taking that into account, a possible area of improvement for this issue is by hiding all the assertions made on the fields of the messages under a user controlled flag.
Description
In summary, this PR hides all the assertions made at the time of construction of messages in rclpy. In order to give the user control whether to do the assertions (and lose performance due to this) or to not do the assertions, all the message constructors have a new argument called
check_fields
which default value isFalse
. This way all the pre-existing code would notice this improvement without the need of doing any modification.