-
Notifications
You must be signed in to change notification settings - Fork 162
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
use parent logger #921
use parent logger #921
Conversation
Signed-off-by: Chen Lihui <Lihui.Chen@sony.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.
I've got a couple of changes suggested inline.
Separately, I'm slightly concerned about the run-time performance hit of this. That is, this function is called for every log message that goes out, and we are now making this function even more expensive for sub-loggers. I'm kind of wondering if we should instead add all sub-loggers to the __logger_map
instead, which means we would use more memory, but keep this function faster. Thoughts?
Thanks for your review.
You're right. It definitely costs more consumption each time while using the sub-loggers not existing. I think we could add all sub-loggers to the |
I think a node is unnecessary to have multiple publishers on a topic named '/rosout', so I will have the publisher shared by the sub-logger. The following is my thought.
Do you have any suggestions? |
I think I understand your proposal; basically, whenever we see a new sublogger in That will work, though it makes Before we go that route, let me ask a question: is the rosout logging layer notified when a new sublogger is created from a parent logger? If it isn't, can we make it so? Being able to do all of this setup work at sublogger creation time would be better, in my opinion. |
Yes.
Sorry for not finding out a better idea based on the current design. Maybe in the future, we could make an implicit node to create a publisher on the topic '/rosout'. (I don't know why ros2 needs to create multiple publishers on topic '/rosout' if there are multiple nodes.)
Could you give me more explanations about |
Going back to the original problem, it seems like the problem comes about in a line like this:
Reading the rclcpp code a bit, it looks like My suggestion is that we add a new function to https://github.com/ros2/rcl/blob/master/rcl/include/rcl/logging_rosout.h called something like |
Thank you for your suggestion. I'd really appreciate that. there is a node named 'node',
RCLCPP_INFO(get_logger().get_child("child"), "Child logger");
RCLCPP_INFO(rclcpp::get_logger("node.child"), "not to publish message on rosout"); Because rcl_add_sublogger was not called before and there is no node named
if RCLCPP_INFO(get_logger().get_child("child"), "Child logger"); and then, RCLCPP_INFO(rclcpp::get_logger("node.child"), "it will use the child logger"); Do you think the cases(2, 3) will make users confused? |
adding dynamic memory allocation and hashmap operation would be issue for logging as mentioned before. in production, logging is really important to make sure that we can have sufficient information w/o overhead.
i think we can call internally based on |
Thank @fujitatomoya, if so, I think the added function |
@iuhilnehc-ynos Hm, good point about the In that case, I think I would propose:
That way, you can avoid allocations at runtime by doing:
But if you forget to do that and just do:
things will still work. What do you think about that? |
Thanks for your proposal. There is something that needs to be confirmed.
so the
I don't think it's what we want because this kind of API (without |
Good point.
OK, I understand this pseudo code.
I don't quite understand this part. Can you rephrase? |
Sorry to make you confused. I thought it seems like an indirect API in RCL because this API need to search a rcl_node_t inside. If only considering the implementation level of Anyway, I'll implement this API as
|
Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>
Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>
Co-authored-by: Tomoya.Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Chen Lihui <lihui.chen@sony.com>
rcl/src/rcl/logging_rosout.c
Outdated
return RCL_RET_SUBLOGGER_ALREADY_EXIST; | ||
} | ||
|
||
status = _rcl_logging_rosout_add_logger(entry.node, full_sublogger_name); |
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.
Use rcutils_hash_map_set(&__logger_map, &full_sublogger_name, &entry)
instead of _rcl_logging_rosout_add_logger
that adding a new publisher.
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Could you help review it in your spare time? Thanks in advance. |
@clalancette friendly ping. |
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
…oyed Signed-off-by: Chen Lihui <lihui.chen@sony.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.
lgtm
@clalancette friendly ping. |
Signed-off-by: Chen Lihui <lihui.chen@sony.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.
Mostly small comments, though there is one question about the underlying data structure.
rcl/src/rcl/logging_rosout.c
Outdated
|
||
|
||
#ifdef __cplusplus | ||
} | ||
#endif |
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 shouldn't be in here; it is only necessary in header files.
rcl/src/rcl/logging_rosout.c
Outdated
status = rcl_ret_from_rcutils_ret(rcutils_hash_map_fini(&__logger_map)); | ||
return status; |
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.
I don't think we want to return the status from rcutils_hash_map_fini
here. Instead, we should return the status from above.
rcl/src/rcl/logging_rosout.c
Outdated
status = rcl_ret_from_rcutils_ret( | ||
rcutils_array_list_add(&__sublogger_names, &full_sublogger_name)); | ||
if (RCL_RET_OK != status) { | ||
goto cleanup; |
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.
If we fail here, we actually should go back and remove the sublogger from the __logger_map.
rcl/src/rcl/logging_rosout.c
Outdated
// remove the entry from the map | ||
if (!rcutils_hash_map_key_exists(&__logger_map, &full_sublogger_name)) { | ||
RCL_SET_ERROR_MSG_WITH_FORMAT_STRING("Sub-logger '%s' not exist.", full_sublogger_name); | ||
__rosout_allocator.deallocate(full_sublogger_name, __rosout_allocator.state); |
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.
Like rcl_logging_rosout_add_sublogger
, we should have a cleanup
label and go there when we fail.
This will also allow us to unravel the cleanup below, so we can goto cleanup
during failures.
rcl/src/rcl/logging_rosout.c
Outdated
@@ -74,6 +75,7 @@ typedef struct rosout_map_entry_t | |||
static rcutils_hash_map_t __logger_map; | |||
static bool __is_initialized = false; | |||
static rcl_allocator_t __rosout_allocator; | |||
static rcutils_array_list_t __sublogger_names; |
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.
I'm wondering whether this should be a hash_map, rather than a list. That way when removing them, we don't have to look through all subloggers to find it, which should improve performance. Thoughts?
rcl/test/rcl/test_logging_rosout.cpp
Outdated
rcl_init_options_t init_options_; | ||
rcl_node_options_t node_options_; |
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.
I'll suggest to remove the trailing _
, just to be consistent with the rest. We aren't totally consistent in tests, but we may as well be consistent within the same class.
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@clalancette |
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
remove the RCL error code Signed-off-by: Chen Lihui <lihui.chen@sony.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.
I left two thoughts inline. Other than those, this is looking pretty good to me.
rcl/src/rcl/logging_rosout.c
Outdated
status = _rcl_logging_rosout_clear_hashmap( | ||
&__logger_map, _rcl_logging_rosout_clear_logger_map_item, &entry); | ||
if (RCL_RET_OK != status) { | ||
goto exit; |
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 is purely stylistic, but I think that we can just return status
here. That is, the extra goto
here doesn't really buy us much. Same goes a few other times below.
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Chen Lihui <lihui.chen@sony.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.
Thanks for the patience and all of the iterations here.
* use parent logger Signed-off-by: Chen Lihui <Lihui.Chen@sony.com> --------- Signed-off-by: Chen Lihui <Lihui.Chen@sony.com> Signed-off-by: Chen Lihui <lihui.chen@sony.com> Co-authored-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
to fix ros2/rclcpp#1694
Signed-off-by: Chen Lihui Lihui.Chen@sony.com