-
Notifications
You must be signed in to change notification settings - Fork 430
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
Service Type Adaptations #2285
base: rolling
Are you sure you want to change the base?
Service Type Adaptations #2285
Conversation
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
We identified type adaptation as the main culprit for the build time increase between Galactic and Humble. |
@alsora could you quantify the time increase? |
@roncapat please refer to this Discourse post: https://discourse.ros.org/t/compilation-bloat-issue/30345/7 It's now too late to revert that commit, as too many things have been built on top of it, but we should not repeat the same mistake and carefully profile this type of changes. |
Clear, thanks. But it is my undestanding (and experience) that:
... so I see the point of trying to optimize before merge, but not to the point of not implementing type adaptation for services and actions anytime soon. Just my two (maybe useless) cents here of course :) |
Absolutely! That's why we should be careful in how we use them.
I would say that this is debatable. Not everyone uses or will use type adaptation, but everyone needs to compile their applications that depend on rclcpp. If we want ROS 2 to be become a mature and adopted software in the industry, we can't compromise on stability or allow major regressions for the sake of adding new features. As a company using ROS 2 in production (iRobot), we can't move from Galactic to Humble due to the aforementioned build time increase, which would cost us huge amounts of money in infrastructure costs (building our software with ROS 2 Galactic takes 1h30m, which become 2h in ROS 2 Humble, with no changes to the source code, just a different version of ROS 2 being linked. Multiply that for hundreds of builds run every day and the $$$ adds up very quickly). The solution will not be to "not do type adaptation", but rather to "implement type adaptation in a different way". However, before derailing this PR too much with "philosophical discussions", we should simply run extensive profiling of the proposed solution and then, depending on the results, evaluate what to do. |
I think we have to be measured in our response here. We can't absolutely prevent new features because it might increase compile times. While that is certainly "a" metric we should keep an eye on, it is (in my opinion) lower on the priority list than other things like runtime performance, user experience, and the like.
And this I totally agree with. If we know how much this increases compile times, then we can make an informed decision on the utility of the change. |
Signed-off-by: CursedRock17 <mtglucas1@gmail.com> finished_service_side Signed-off-by: CursedRock17 <mtglucas1@gmail.com> Small Reversion Signed-off-by: CursedRock17 <mtglucas1@gmail.com> More specialization Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Sorry for the delay, but I've got a solid chunk of the altered design implemented.
These were just quick build times from my local machine so they should be taken with a grain of salt.
|
Branch | Min (sec) | Max (sec) | Mean |
---|---|---|---|
Service_Type_Adapt | 188.94 | 219.26 | 199.90 |
Rolling | 174.87 | 200.63 | 192.57 |
That 7.33 seconds puts us at an increase in build time at 3.8% increase, certainly better that the ~18% jump from the original implementation of Type Adaptions, but still an increase, even though we expected it. Though any other additions in the methods that we change, may increase these compile times because they're so reliant on templates.
Runtimes
Another issue that may arise are runtimes, while most of these services are passed by some form of shared_ptr
it seems that PR #2199 and this repository highlight the slowing of templating functions. These may not be directly related to this, but I figured it's best to acknowledge that damage could be inflicted. That being said, I didn't do any testing related to this so I cannot be perfectly sure how that will change.
Design Differences
If you take a look at test_client_with_type_adapter you'll notice how I changed from making a structure which takes in the request
and response
to just taking in the whole service
.
struct rclcpp::TypeAdapter<CustomBool, rclcpp::srv::SetBool>
Additionally, the custom structures now mimick services:
struct CustomBool
{
struct SetBoolResponse
{
bool success;
std::string message;
};
using Request = bool;
using Response = SetBoolResponse;
};
We also changed from converting to custom_type
or ros_type
, instead we can convert to a ros_service_request
, ros_service_response
, custom_service_request
, and custom_service_response
i.e:
static void
convert_to_ros_service_request(
const custom_type::Request & source,
ros_message_type::Request & destination)
{
destination.data = source;
}
The reason this occurred, was confirmed by my doubts earlier, we cannot create mimickable service requests and responses. There isn't type support to enable each Request
and Response
to be a message, while allowing the custom Service
to be a service. Since ROS works by generating the Service through ROSIDL we would have to do that for each custom Service Request and Response by building a fake Service, which can't happen since we have no location information. The user would also have to override all the type traits like is_message
for each each response and request, which might allow incorrect styles of code to pass through. I hope that I didn't explain that too incorrectly or weird.
While the user does have to set up the Adapted Type Structure a bit differently, this does allow us to keep it more concise, gives us more control over the types, and should work better this way. A slight issue I should address is naming conventions, really just using ros_message_type
could become ros_type
since that's how the custom side is already set up. This way we don't have to alter the TypeAdapter
class.
Callback Mixing
As of right now the entire Service
side would be ready to go, with the able to call any combination of custom and ROS alongside request and response. Though, on the Client
side, I'm trying to get this implementation just right, right now we can only support callbacks which totally share their type with the request or response. For example
async_send_request which has both a request and some form of callback with a request type. The actual implementations don't work because of template specialization, the compiler (in my case Clang) recognizes that the function signature would be the same, although it wouldn't seem to be the case. Past these weird implementation details if we did get it to work, our callbacks cannot have different types than our promises.
To extend of that, converting between futures seems difficult to since many of them start out as empty and returning a different type than the user wants(i.e custom when they want ROS) is not something we want to do.
For now though, a lot of our cases are covered. One last thing that does come as a result of all these different cases is template specialization. While the compile times didn't increase absurdly, there is quite a bit of code that could be seen as bloat for those trying to work on the codebase and decipher where their types are coming from. There was a large increase in the original pull request that added adaption, but this would be another one, and this would be a repeated process if we add Type Adaptions for actions as well. I am looking for optimizations and places that we can remove code, such as conversion for futures, but in the mean time this would be a large piece of templated and specialized code.
@CursedRock17 thank you a lot for the analysis, I will need to take some time to digest it and give a more meaningful feedback. What these type of changes really affect is not the build time of rclcpp (which yes, it is affected, but this is IMO a minor and unavoidable problem because you are adding new features), but rather the build time of C++ applications using rclcpp. What's important to minimize is the impact on those users that don't care about the new features being added. We can also measure the impact when switching from the standard service type to type-adapted ones |
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Ok, I went and did some testing using ClangBuildAnalyzer and this repository if needed to recreate. During the testing I found that the previous iteration that I had didn't even work, as After fixing those previous errors I have acquired the following testing results from ClangBuildAnalyzer
**** Time summary:
Compilation (2 times):
Parsing (frontend): 2.7 s
Codegen & opts (backend): 0.3 s
|
@CursedRock17 i am not sure if you are still working on this. Just FYI, service type support part has been already implemented by #2358 during |
Yeah I'm still working on this pull request, it just adds quite of bit of an increase to compile time, so I'm looking to bolster my template metaprogramming skills to increase efficiency, without sacrifing features in order to get the time to compile down. As for the service type support I did see that #2209 resolved type support issues so I should be able to remove get_type_support_handle.hpp |
This pull request is a larger one in response to issue #1666 which was to add Type Adaptations to Services/Clients, Type Adaptation were originally added to rclcpp with PR #1557 in 2021. Type Adaptations were a whole design document, which is what I tried to follow when implementing this for services and clients, If we're able to do this we can also have a strong resource for the Actions side of things.
At the original time of this PR you can see it doesn't work, I wanted to figure out the direction we needed to take with how we implemented the Type Adaptations. In the documentation, the given example was:
where the original thought was to just make a
struct
that replicates a service with a Request and Response, that's what I applied. There are two spots, currently where this doesn't work -get_service_type_support_handle
These issues highlight the difficulties with using the strategies in the current design docs, which is: the proxy struct that you pass to be a service isn't actually a service. In the case of the rosidl_typesupport, currently this generated method was what I found allowed any services to pass through with this:
being passed as the type, afaik there is only one to change this and enable it as a message which where
GetTypeDescription__C
comes into to play, it creates a different method:calling
ROSIDL_GET_SRV_TYPE_SUPPORT
. Circling back toget_service_type_support_handle
, we don't where the user is going to get these services from, at least I don't think, so that we could apply this type. Then, GetTypeDescription__C doesn't pass to the Service it's used in because, theRequest
andResponse
aren't technically messages again, they are the generated type that the compiler deciphers, this was all added in the Type Description design and creation. There is also a similar version of type support in rosidl_runtime_cSo I submitted the pr early, to try and gain some sort of insight that I could just be missing and find out the direction we want to go. Technically speaking, we could work by creating another TypeAdaptation
struct
that could take in some form ofAdaptedStruct
and just do what TypeAdapter does know, but with Response and Request, but this isn't very effective because we're generating way more code, we'll have to do it with 3 things when we add Actions, and I don't even know if it will work entirely. Let me know what direction we want to take on this, anything I missed, and if anything needs to be cleared up