Initial commit of primitive_array type #34
Conversation
There was a problem hiding this comment.
General feedback: This is now getting to the point where it's difficult for me to "simulate" the codepath in my head as I review the PR. I think in cases like these, I'd prefer we write some tests to confirm this logic will continue to work after future changes.
lib/ros_value.h
Outdated
| } else { | ||
| primitive_info_.~primitive_info_t(); | ||
| } | ||
| } | ||
| RosValue operator=(const RosValue&) { | ||
| throw std::runtime_error("Can't call this"); |
There was a problem hiding this comment.
I feel like we should have a better error message than this
There was a problem hiding this comment.
I actually want to write this, thanks for catching.
@brendangeck I can definitely add some more tests around arrays specifically. The existing tests do a decent job of covering the |
Fixes: DATA-556
Description of Changes
Using pre-allocated
RosValues to hold each primitive within an array can be very expensive for large arrays of small data types.This change makes it so that we create a RosValue on the stack to point to an element of these arrays whenever the items are accessed. This involves a few steps:
RosValue::Pointers now have two 'underlying definitions': one where we track the location in the dynamic vector, and one where we simply store aRosValueprimitive_arraytype which stores primitives (not including strings) which do not haveRosValues pre-allocated for them.Test Plan
Verified that a large amount of memory was no longer being used by reading a bag full of CompressedImage messages.