Simplifying the SET API and its variants

[housengw]

While working on issue 44, I realized it might help with ease of use if we provide an alternate set of SET APIs that are defined based on the memory properties of the set data.

Design

Instead of adding destructor variants to existing SET APIs, I propose a design that will only have two SET variants, one for static data and one for dynamic data:

1. SET(out, val) (implies static data)
2. SET_DYNAMIC(out, val, destructor) (implies dynamic data)

Implementation:

• SET(out, val) is the same as the current SET.
• SET_DYNAMIC(out, val, destructor) is the same as SET_ARRAY except without the elem_size argument (which SET_ARRAY doesn't use in its macro body, anyway) and have an extra field for the destructor.

Example use case:

Suppose is a struct called data:

1. if data is static (none of its members is malloc'd), just use SET(out, data)
2. if data has dynamic members, the user would malloc the whole struct, then malloc the members. Then, use SET_DYNAMIC(out, data, &destructor), where void destructor(void* data) is a function provided by the user that takes data and frees every dynamic member of it as well as data itself. If destructor is NULL, free is used instead.

The point of this API is to simplify memory management and reduce the number of assumptions the user has to make to use the dynamic memory SETs.

EDIT:
Action Items:

• remove the elem_size argument from SET_ARRAY since elem_size is already set in the default token generated by deferredCreateDefaultTokens in CTriggerObjectsGenerator.java
  • remove elem_size from _LF_SET_ARRAY and SET_ARRAY
  • remove elem_size argument from API call in SetArray.lf
• Implement the SET_DYNAMIC API.

[edwardalee]

As we just discussed in person, the decision is to define SET_DYNAMIC with three arguments, a port, a void* pointer, and function pointer to a destructor. (No length or element_size argument). If the destructor pointer is null, we will use free().

The element_size argument in SET_ARRAY is redundant, so we will eliminate that.

[Soroosh129]

Great! I like this proposal.

I think @housengw had found a way to use C's va_arg mechanism (e.g., SET(out, ...)) to overload the SET macro for both SET and SET_DYNAMIC. I think this looks cleaner, but potentially could be confusing to see in C.

What do you think?

[housengw]

I think that since the third argument is a function pointer, we should use difference names for the macros to be safe..

[lhstrh]

As you are working on this, can I suggest to take the opportunity to switch to lowercase names as brought up in #40?

[housengw]

That's a good point. I am open to the change. @edwardalee @Soroosh129 What do you think?

[edwardalee]

That is OK with me. Do we want to leave the upper case names there for backward compatibility? I don't see any harm in that...

[Soroosh129]

It's OK with me too.

We can keep SET in a deprecated state and eventually remove it.
We can include a compiler warning in the definition of SET that informs the programmer of this deprecation.

[housengw]

I couldn't get the solution outlined in @Soroosh129's link to work with macros. So I used _Pragma instead.
https://docs.microsoft.com/en-us/cpp/preprocessor/pragma-directives-and-the-pragma-keyword?view=msvc-170

[housengw]

a lower case set macro seems to conflict with a library function in MacOS (Github Action Output):

/SDKs/MacOSX12.1.sdk/usr/include/c++/v1/istream:163:
    In file included from /Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/c++/v1/ostream:141:
    /Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/c++/v1/bitset:703:17: error: too few arguments provided to function-like macro invocation
        bitset& set() _NOEXCEPT;
                    ^
    /Users/runner/work/reactor-c/reactor-c/test/C/src-gen/target/HelloWorldCCPP/ctarget.h:69:9: note: macro 'set' defined here
    #define set(out, val) _LF_SET(out, val)

[Soroosh129]

This error is limited to the CCpp target it looks like.
However, in retrospect, set can easily clash with existing definitions (same argument can apply to SET).
Perhaps, the correct name would be lf_set.

[lhstrh]

I'm not sure I like this solution... Is this really necessary?

[housengw]

I think so. Even if we can circumvent name conflicts with set for now it will likely come up again in the future.

[housengw]

After a discussion with @edwardalee, a conclusion was reached that a copy constructor is needed for mutable inputs. It makes sense add a copy_constructor field to lf_token_t. However, one case still bugs my mind: in the remote federated case, the serializer also has to provide the copy constructor and destructor using SET_DYNAMIC right?

[Soroosh129]

It might not be enough to add a copy_constructor field to lf_token_t because tokens get recycled and the copy constructor will be lost. Is the proposal to set the copy constructor every time we want to produce data on an output port or to also add a copy_constructor field to the port and pass it on to the underlying token when needed?

Also, it might be strange to assign the copy_constructor in the sender. The sender has no idea if the input port at the destination is mutable or not. Is the receiver supposed to set the copy constructor then? In that case, this wouldn't be a problem for federated execution.

[Soroosh129]

For federated execution in general, I think it's better to disallow (or strongly deprecate) token types altogether and only allow the default serializer to be used for plain old data (POD). It seems to me that allowing users to potentially send pointers over the network is error-prone. If we improve our Protobufs integration, this shouldn't be too much of a pain for programmers.

This strategy will also make it more straightforward to enable Python and C federates to talk to each other (even if they were not necessarily designed to do so). CPython provides built-in functions to convert PODs between C and Python. For more complicated types, Protobuf can take care of the serialization.

[housengw]

After meeting with @Soroosh129 and @edwardalee, we have come to the following design:
destructor and copy_constructor should be part of the port. We will have APIs to set destructor and copy constructor on the port instead of a set that takes the destructor and copy constructor as arguments.

Implementation:

1. Add token field to the port struct (change generateAuxiliaryStruct:CPortGenerator.java).
2. Add copy_constructor field to lf_token_t
3. Add APIs SET_DESTRUCTOR(port, destructor) and SET_COPY_CONSTRUCTOR(port, copy_constructor).
4. Remove the SET_DYNAMIC API.
5. In SET, check if the port has a token. If not, perform set using the current SET logic. If so, perform the set using the SET_DYNAMIC logic.

[housengw]

closed by #68

[petervdonovan]

For how long will we need to keep the deprecated API functions around?

For context,

• the C runtime is unhealthy and I would like to work on cleanups;
• we just put out a release, so there should be ample time before things break for our (hypothetical?) end users;
• our major version number is 0; and
• if memory serves, I think Professor von Hanxleden advised against maintaining deprecated APIs when one isn't sure whether anyone is using those APIs.

[Soroosh129]

I think now is as good a time as any to remove them.

[lhstrh]

Yes, now is a good time :-)