Allow copy-on-sent semantics for ProcessArrays

[smarsching]

At the moment, ProcessArrays always have move-on-sent semantics, meaning that sending a value moves it from the sender to the receiver. After having been sent, the value cannot be accessed by the sender any longer. A user can disable swapping and then access the sent value with the getLastSent() method.

This solution has several disadvantages:

1. It makes the API more complex because the receiving side has to check whether the process variable is actually swappable before trying to swap the array.
2. It makes the API more complex because one has to use get() for ProcessScalars on both the receiver and the sender side and for ProcessArrays on the receiver side, but getLastSent() for ProcessArrays on the sender site.
3. It does not work nicely with bi-directional process variables (see issue #2).

For these reasons, we should always allow swapping of arrays and get rid of the getLastSent() and isSwappable() methods. Instead, we should add a copyOnSend flag that can be enabled when creating a ProcessArray. When this flag is enabled, the send() operation makes a copy of the array that is then transferred to the receiver while the original stays with the sender and can thus safely be accessed after the send operation.

This copyOnSend flag should remain optional (though we should consider setting it by default), so that large arrays only transferred in one direction and not needed on the sender’s side after sending can be transferred without incurring the costs of a copy operation. For obvious reasons, bi-directional process variables must always copy the array when sending it, so that for them the flag is not optional, but mandatory.

[mhier]

I would prefer having two different send() functions instead of a copyOnSend flag for two reasons:

• in a complex application, it might be difficult to understand which calls to send() copy and which move,
• one might want to decide this from send to send operation, which is not possible if this flag needs to be passed when creating the ProcessArray.

[smarsching]

I do not think that there is a use-case for sometimes copying and sometimes not copying the value of the same process variable. There is only one case when copying on send makes sense: When the process variable it not write-only but also used for read access. In this case, copy-on-send is necessary in order to avoid reading a stale or uninitialized value. It is very unlikely that I can decide at the point where I send a process variable whether other parts of the code might read it after the send operation.

For this reason, having send() and copyAndSend() does not make sense to me. When I want to read from a process variable after sending it, I always have to use copyAndSend() and (accientally) using send() would lead to undefined behavior.

For example, consider a case where originally a ProcessArray is never read but only written. In this case, one might want to disable copy-on-send for performance reasons. Now, one makes changes to the application which make it necessary to also read from that same ProcessArray. When having separate send methods for copy and non-copy, one would have to identify all places where the process variable is sent and change them from send() to copyAndSend(). When having a flag, one would only have to change this flag in a single place (where the process variable is created) and could be sure that it is now safe to read from the process variable.

More importantly, one always needs copy-on-send for bi-directional process variables. Bi-directional process variables are always read on both sides (if they were not, there would be no reason for them to be bi-directional). This means that one would always have to use copyAndSend() for bi-directional process variables.

Last but not least, having two different methods would make ProcessArrays and ProcessScalars more asymmetric than they already are: For ProcessScalars, we always use copy-on-send, so they would not have the copyAndSend() method.

When encoding the copy-on-send behavior in a flag (and enabling copy-on-send by default), one gets a very nice and easy to understand behavior: ProcessArrays and ProcessScalars behave in the same way in that they allow accessing their current value before and after send or receive, regardless of their type (sender, receiver, or bi-directional). Only when explicitly disabling copy-on-send (which one would probably one do for large arrays that are sent frequently), one gets slight different semantics. This means that the simple, expected behavior is the default and one only has to be careful when tuning for performance.

For these reasons, I strongly advise against using different methods for sending with and without copying. If we want to go that route, I at least recommend making copy-and-send the default and having a special method (e.g. sendWithoutCopy()) that does not copy the value. However, I still think that encoding this in a flag makes much more sense.

[mhier]

I don't like functions which have quite different behaviour depending on some flags set potentially in a completely different part of the application. There may be many different use-cases for keeping a local copy after sending. For example, a large array generated from a few user-input parameters will be re-generated only if the input changes, but might be needed by some algorithm periodically. Since ideally the application should store the generated values directly into the process variable (and not copy it there from some other buffer), the algorithm will have to read it from the process variable.

I agree, sending with copy should be the default, whether we decide for two functions or for the flag. The destructive send is not a behaviour one would "naively" expect. Exactly this is my point against the flag: a reader of the code who did not completely write it has little chance to understand that some calls to send() make a copy and others don't, e.g. if the process variables are created in a different source file. I think this will go wrong sooner or later. If the variable is frequently sent with similar values, the errors will be very subtle and hard to understand (since reasonable values will be in the buffer after the first few transfers). We already had this issue with the current implementation, so I would like to not make the same mistake again...

When having two functions, e.g. called send() and sendDestructively() (or similar), ProcessArrays and ProcessScalars don't have to behave seemingly different. Both implementations will have both functions, just for the ProcessScalar they will do the same thing. By definition, you cannot rely on reading the old value from the ProcessScalar after calling sendDestructively(), but it doesn't hurt if you actually still could. This is simply a hint, which can optionally be used by the implementation for optimisation. In future after unifying the interface with DeviceAccess, other implementations might even do similar optimisations with scalars, or may not be able to optimise like this at all (for all current DeviceBackends there is no point in this kind of optimisation, the data is anyway copied).

Besides, since we need to have a unified interface between scalars and arrays (as DeviceAccess already has), also the solution with the flag must be applied to both the ProcessArray and the ProcessScalar. The ProcessScalar would just ignore it.

[smarsching]

Thank you very much for your comments. I think we are getting closer to a solution.

We already agree that "copy on send" should be the default. Regarding the issue of how to distinguish between the two possible ways of sending data, I think that we basically worry about the same thing: It should be clear to the user when a process variable is sent destructively and thus cannot be read any longer.

I agree that having different behavior for the same function can be irritating and that in the optimal case the send behavior should be specified in the same part of the code where the process variable is read. You are worried that a process variable might be created in one place and used in another place. I am worried that a process variable might be written in one, but read in a different place.

You are right, that copy-send and move-send should be different methods. However, I would still like to have some protective measures to avoid using move-send on a process variable that is also read. For example, one could set a flag when creating the process variable, that results in move-send throwing an exception. This way, one could be sure that no code accidentally uses move-send when it should use copy-send. The best solution would be if we could actually catch reading the variable after doing a move send, but I do not see a viable solution how we could do this. The get() method may legally be used to obtain a reference for writing, so throwing an exception from get() when the process variable’s value has been moved away will not work.

Regarding the name of the move-send method, I still have not found one that I really like. I like about the proposed sendDestructively() that the name already contains a warning about the danger associated with using this method. However, I do not like that it is not clear in which way calling this method is “destructive”. I am considering sendMove() because the semantics are so close to C++’s std::move and any C++ developer should immediately realize the consequences. However, this name has the disadvantage that the effects of std::move apply to the argument, while in our case they apply to the object we invoke the method on.

For now, I am going to implement only the basic changes inside a branch, using either method name (or even a different one if a better one crosses my mind). Changing the name of the method is not a big deal, so we can decide on the method name later, before merging into the master.

[mhier]

You are worried that a process variable might be created in one place and used in another place. I am worried that a process variable might be written in one, but read in a different place.

I think, having the creation separate from reading/writing/sending the process variable is much more common in a well structured code.

I already have the ApplicationCore in mind, where we define modules and the process variables are (in a sense) created as member variables of the module. The necessary information is passed using a brace initialiser, so this may go into the header file, while the actually access will be in the c file.

On the other hand, if you have the send() and writing and reading the process variable in different places, you probably almost always will have the creation in a different place, too. So I do not see much of a difference here, whether the information goes into the creation or into the call to send().

However, I would still like to have some protective measures to avoid using move-send on a process variable that is also read. For example, one could set a flag when creating the process variable, that results in move-send throwing an exception.

I would prefer it the other way round, i.e. to have an optional flag, which has to be set when allowing the move/destructive send. The default is not to allow it, since it should be used only when really needed.

Regarding the name of the move-send method, I still have not found one that I really like.

I am find with either name, sendDestructively() and sendMove(), with a slight preference to sendDestructively(), since, as you mention, it contains a stronger warning. There is always a documentation which one can consult in case of doubt what is actually being destroyed or moved. What is more important is that one is not easily mislead to a different meaning. I see no other possible thing which might be destroyed or moved (apart from maybe the entire process variable, which would be very obviously pointless), so I don't think this is a real danger here.

[smarsching]

I have now implemented the changes as discussed in the issue-5-v1 branch. @mhier: Maybe you can review them. I guess we should wait with merging these changes until @killenb has merged his changes (although I do not expect too many conflicts).

[mhier]

The changes look fine to me, thanks!

I only now wonder, if it would be better to add some specially-typed flag instead of a boolean, since it is not extensible (in case we need more flags later) and difficult to understand in the source code ("what does that true mean there?"). I would suggest introducing something similar like the AccessMode flags in DeviceAccess: https://github.com/ChimeraTK/DeviceAccess/blob/master/device/include/AccessMode.h

This can be done of course in a second step later, when there is time (i.e. I can do it if you don't have time).

[killenb]

I just merged the issue-5-v1 branch into the master. I will continue the DeviceAccess unification including these changes (needs some manual merge work which I want to do as soon as possible).

[smarsching]

This sound like a good idea. I can implement the changes suggested by @mhier later, after you have merged your changes.

[smarsching]

I think this issue has been resolved as part of the integration of DeviceAccess: The write(…) method copies and the writeDestructively(…) (I do not really like the name, but I do not have a really better idea either) moves the data. By having two different methods, there is no need for a flag any longer.

For this reason, I am closing this issue.