Skip to content
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

Cleanup error reporting in the type hash code. #1084

Merged
merged 1 commit into from Aug 3, 2023

Conversation

clalancette
Copy link
Contributor

In particular, using the RCL_CHECK_* macros in both rcl_node_type_cache_register_type() and
rcl_convert_type_description_runtime_to_msg() meant that we would overwrite the error message of the latter in the former. This would end up with an rcutils warning about losing error messages

Avoid that by just doing an open-coded check for NULL if rcl_node_type_cache_register_type(), passing along the error we got from the lower layers.

While we are in here, also very slightly revamp the client_init code to not have two separate calls to type_support->get_type_hash_func().

In particular, using the RCL_CHECK_* macros in both
rcl_node_type_cache_register_type() and
rcl_convert_type_description_runtime_to_msg() meant that
we would overwrite the error message of the latter in
the former.  This would end up with an rcutils warning
about losing error messages

Avoid that by just doing an open-coded check for NULL
if rcl_node_type_cache_register_type(), passing along the
error we got from the lower layers.

While we are in here, also very slightly revamp the
client_init code to not have two separate calls to
type_support->get_type_hash_func().

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Contributor Author

I should also say that this is the first in a large-ish series of patches that will reduce rcl test time substantially. Locally, with this series in place, my test time went from ~8 minutes to ~5 minutes.

@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I'm surprised that the error message handling incurs such overhead. Thanks for fixing it!

@clalancette
Copy link
Contributor Author

+1 I'm surprised that the error message handling incurs such overhead. Thanks for fixing it!

Ah, I should have been more clear. The error message handling code does incur a bit of overhead, but that wasn't the main thing that allowed me to remove time. This (and a whole host of other patches to come) were mostly just things I noticed while shrinking the time down.

@clalancette clalancette merged commit d7a51b5 into rolling Aug 3, 2023
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the clalancette/type-hash-errors branch August 3, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants