Dynamic Subscription (BONUS: Allocators): rosidl

[methylDragon]

See: ros2/ros2#1405

[wjwwood]

one comment, otherwise lgtm

[wjwwood]

It's not clear to me if you should deallocate the hash_map in this case. I think you might need to. Otherwise it's a guaranteed memory leak, because you didn't deallocate it, but it also is no longer accessible after this.

[methylDragon]

Well, in this case it isn't used anywhere else, so I think it's okay?

I did this because valgrind was complaining

[wjwwood]

Maybe we're not talking about the same thing. Do you mean that if you deallocate hash_map here (before return ret;) that valgrind was complaining?

[methylDragon]

Ah, I meant that I added the deallocation because valgrind was complaining about the leak

The hash map isn't accessible from outside the function, and deallocating it doesn't cause what it points to to get deallocated, so I don't think there are any issues

[clalancette]

As far as I can tell, this is a correct bugfix. That is, rosidl_runtime_c_type_description_utils_get_field_map allocates and adds values to a hash_map, which is the responsibility of the caller to free. Further, the hash map stores a key -> value mapping of char * -> rosidl_runtime_c__type_description__Field *. That is, we are only getting the pointers to the underlying data, not the underlying data itself. Thus, it is entirely appropriate for this function to initialize a hash_map to NULL, have that allocated and filled in by rosidl_runtime_c_type_description_utils_get_field_map, use that data, and then free it.

That said, this is buggy. If we end up at line 693, then we'll skip cleaning up the hash_map. I'll suggest moving this above the if (description->fields.size != map_length) { block, since we already got the data we need from the hash_map.

Finally, this actually seems like a very expensive way to get the number of unique items in the type description field. It would be far more efficient to have a rosidl_runtime_c_type_description_utils_get_size which iterates over the individual descriptions and just returns a count. @methylDragon you don't need to fix this now, but it would be good to open up an issue for follow-up which includes comments like this.

[clalancette]

Oh, you know, never mind my comment about this being buggy; the end case was actually handling this. Still, I think if we move this block up above, we can avoid having this in two places.

[methylDragon]

I'll put opening up a followup issue on my docket for now!

[methylDragon]

Regarding rosidl_runtime_c_type_description_utils_get_size, we would still need to check for duplicate individual descriptions, which I think would end up needing a hash set, or a growing list of individual description names that we check against as we're building the count (or we sort it and check for neighbouring duplicates?)

I ended up going with the hash set method (with the hash map construction), but yes we can defer this

[clalancette]

This comment is still open about getting rid of the duplicate rcutils_hash_map_fini. But it is correct right now, it is just code that is unnecessary. So we can defer that to a follow-up as well.

[clalancette]

As far as I can tell, this is a correct bugfix. That is, rosidl_runtime_c_type_description_utils_get_field_map allocates and adds values to a hash_map, which is the responsibility of the caller to free. Further, the hash map stores a key -> value mapping of char * -> rosidl_runtime_c__type_description__Field *. That is, we are only getting the pointers to the underlying data, not the underlying data itself. Thus, it is entirely appropriate for this function to initialize a hash_map to NULL, have that allocated and filled in by rosidl_runtime_c_type_description_utils_get_field_map, use that data, and then free it.

That said, this is buggy. If we end up at line 693, then we'll skip cleaning up the hash_map. I'll suggest moving this above the if (description->fields.size != map_length) { block, since we already got the data we need from the hash_map.

Finally, this actually seems like a very expensive way to get the number of unique items in the type description field. It would be far more efficient to have a rosidl_runtime_c_type_description_utils_get_size which iterates over the individual descriptions and just returns a count. @methylDragon you don't need to fix this now, but it would be good to open up an issue for follow-up which includes comments like this.

[clalancette]

This comment is still open about getting rid of the duplicate rcutils_hash_map_fini. But it is correct right now, it is just code that is unnecessary. So we can defer that to a follow-up as well.