[rosidl_generator_cpp] ZERO initialization of messages

[mikaelarguedas]

I think it's a bug but would like to confirm:

If I define foo.msg:

bool[] foo [true, false, true]

And chose MessageInitialization::ZERO, foo should be allocated and populated exactly the same way as if I had

bool[] foo

right ?

Currently I get a vector of size 3 populated with zeros (same thing for bounded vectors)

[clalancette]

Well, the current behavior is by design (so not technically a bug). Whether you agree with that design is a different story :).

My thinking behind ZERO initialization is that it does all of the same things that ALL does, except that instead of placing the default values into place, it places zero (or the equivalent) into place. Thus, in your above example, ALL would have made a 3-byte vector, and filled it with [true, false, true]. Using ZERO instead would still create a 3-byte vector, and fill it with [false, false, false] (ignoring bugs like #250). If you wanted the behavior where you got a size-zero vector, then you would use SKIP, which would indeed not populate that vector (though it also doesn't populate anything else either, which may not be what you want).

There are 2 different ways we can go if you think we should change this behavior. We can either redefine the design of ZERO so that it doesn't expand the vectors at all, or we can add a new initialization type to not expand vectors. There is a (slight) downside to redefining the design of ZERO, which is that you don't technically get "zeros" in-place anymore, and instead you get "zero-size" vectors. The distinction is that in the current code, with your example above, a user could reasonable do:

msg::foo x(rosidl_generator_cpp::MessageInitialization::ALL);
x.foo[0] = true;

And either use rosidl_generator_cpp::MessageInitialization::ALL or rosidl_generator_cpp::MessageInitialization::ZERO with no further code change. If we change the semantic, then that x.foo[0] would no longer work, and the code would have to be slightly different.

All of that being said, it is pretty minor overall. I don't have strong feelings about the semantics of ZERO, so if you think we should change it, we can certainly do that.

[mikaelarguedas]

ok, I guess that's my misunderstanding of what we meant by ZERO. I had the feeling that this would initialize all the fields to "our" defined defaults (that is empty arrays for non static ones).
So I didn't expect the allocated memory and the value population to depend on the defaults provided and that the defaults would just be ignored. At most I would have expected the bounded arrays to preallocate all their memory and the dynamic ones to not allocate at all.

But I understand the point about making it match the memory that would have been allocated if ALL was used 👍 .

This could be used when the user doesn't want the overhead of potentially complex / large default values (since they are overridden anyway) but still wants to ensure that all variables are properly initialized.

I assumed that the goal here was to avoid a overhead of assigning many values (that's how I interpreted "large" here as all rosidl types supporting default values are of fixed memory size).
I'm not sure how much of a performance hit we avoid by replacing:
x = {1, 2, 3, 4, 5, 6, 7, 8, 9, ..., N} by x = {0, ...N-2times..., 0}

But if it's what was decided during design I'm fine keeping it.

[clalancette]

I assumed that the goal here was to avoid a overhead of assigning many values (that's how I interpreted "large" here as all rosidl types supporting default values are of fixed memory size).
I'm not sure how much of a performance hit we avoid by replacing:
x = {1, 2, 3, 4, 5, 6, 7, 8, 9, ..., N} by x = {0, ...N-2times..., 0}

But if it's what was decided during design I'm fine keeping it.

There may or may not be a performance benefit to assigning all zeros vs. assigning numbers (in theory, assigning zeros can be faster, but it depends a lot on how C++ does the initialization, what is optimized on the OS, and what processor architecture you are running on). That being said, I may have missed that the goal of ZEROS was to avoid some of this additional overhead, which is why we ended up with the design we did. As it stands, the two initializers that avoid overhead (in most cases) are SKIP and DEFAULTS_ONLY.

All of that being said, if we are going to keep the current design, it may be worthwhile to update http://design.ros2.org/articles/generated_interfaces_cpp.html#constructors to make this a bit clearer. If you think that is a good idea, I'll go ahead and do that.

[mikaelarguedas]

Yeah maybe clarify that ZEROS means: allocate as if ALL and then zero-initialize the memory.

How I saw it when reading the design originally (when I talk about allocation I'm focusing on fields that don't have a fixed size):

• SKIP: allocate only the minimum required for each field and don't initialize anything
• ALL:
  • for members with defaults specified:
    • allocate what is necessary to fit the default values and initialize the storage with the provided default values
  • for member without default values: allocate only the minimum required for each field and initialize with our default (that would resultin an empty array for bounded/unbounded arrays)
• DEFAULT_ONLY:
  • for members with defaults specified:
    • allocate what is necessary to fit the default values and initialize the storage with the provided default values
  • for member without default values: allocate only the minimum required for each field and don't initialize them
• ZEROS:
  • for all members regardless of if defaults are provided:
  • allocate only the minimum required for each field and initialize to our specified defaults

[mikaelarguedas]

All of that being said, if we are going to keep the current design, it may be worthwhile to update http://design.ros2.org/articles/generated_interfaces_cpp.html#constructors to make this a bit clearer. If you think that is a good idea, I'll go ahead and do that.

I'm not sure we'll have time to confirm with the team that my reading is the one we want to implement as well as adapting the implementation so I think that clarifying the behavior in the design is a good path forward for today 👍

[wjwwood]

I also find this behavior weird, or at least didn't expect it.

For me, a bounded array or unbounded array need to be filled with information, so if I don't want the defaults in them, then they would be empty. Consider something like:

string[] preferred_names ["default_1", "default_2"]

If I choose zeros then I'd expect there to be no defaults given, personally. OTOH:

string[3] top_3_choices ["foo", "bar", "baz"]

I would expect the array to contain three empty strings on zero.

Maybe it doesn't matter that much, since I expect zero to be an uncommonly used thing (I think the default, ALL will be overwhelming popular), but it is surprising to me.

[clalancette]

Maybe it doesn't matter that much, since I expect zero to be an uncommonly used thing (I think the default, ALL will be overwhelming popular), but it is surprising to me.

Yeah, I think that is definitely the case. To be honest, I really only understand the need for ALL (default, safe) and SKIP (performance); the middle two don't seem all that useful to me. They were just relatively easy to implement. For now, I'll update the documentation, and I think it will be safe enough for us to change this semantic later if we want to.

[dirk-thomas]

I agree with @mikaelarguedas concern. Imo ZEROS does not consider the default values and only makes sure that all fields have a defined value. Therefore dynamic arrays should have no elements (since the number of elements would be inferred by the default value which the ZEROS option aims to avoid).

Since the required change to address this bug I would suggest to rather address this instead of changing the design description for now and then change it back in a future release.

[mikaelarguedas]

ok we seem to have consensus on the expected behavior of that option so I'll label this as a bug ready to be worked on rather than a question

[clalancette]

This was fixed by #254