Re: Updated dataflow semantics for RTT

Hi Johannes, thanks for the long explanation.

On the implementation side, this whole discussion revived an idea I
have had a while ago, and is pretty much in line with the discussion
Herman started.

Basically, one could get rid of (a.k.a. "deprecate" of course) the
connection API on the ports themselves, and replace it by a separate
object (let's call it "ConnectionManager") which would be in charge of
managing all these buffers and create all these connections and
implement the . In its simplest form, each process would have one of
these objects.

The basic operations it would have support are:
 - create/remove storage object (data object / buffer object)
 - connect output port to storage object
 - connect storage object to input port

A "pull" connection would involve the ConnectionManager of the output
port. A "push" connection the ConnectionManager of the receiving side.
How one gets shared or private (or mixed) connections is obvious.
Flags would be split between the two halves (which is good !). For
instance, 'init' only affects the output->buffer side.

In principle -- but that's a principle that even I take with a big
grain of salt given that I did not seriously touch that stuff for
years -- the current underlying API (creation of the two channel
halves) would be reused. In other words, only where the connections
are created would change, not how they are created.

On top of that, it's trivial to create higher-level, easier-to-use
APIs, and also implement the current connection API so as to not break
existing code.

It would open the path to a lot of interesting things:
 - finally have a decent way to monitor the data objects (i.e. buffer
fill rates, "data loss", data counters, that kind of thing). It would
also make obvious how one could replace the RTT "base connection
manager" by something else without touching the components. It would
spell how wonderfully relevant RTT still is to those who don't see it
;-)
 - if we add the possibility of inter-ConnectionManager connections
would allow us to move the keepSample functionalities of OutputPort
out of it, which would also make the whole init functionality a lot
more transparent. It would allow to expose (and monitor ...) the CORBA
dispatchers.

Now, I do understand that it might sound "blue sky" and that you might
not have the time for that ... just sayin' ;-)

Sylvain


2015-09-28 16:52 GMT-03:00 Johannes Meyer <[email protected]>:
> Hi,
>
> thanks for all your input and ideas and I agree with Herman that the
> "one-framework-for-everything" approach of RTT is probably not how you would
> start a new project nowadays. But I would like to support Stephen's call to
> focus this discussion on the technical aspects of how to improve the
> existing implementation in as small as possible ("incremental") steps that
> add support for some new use cases that are not covered yet (but have been
> partially covered by v1).
>
> Maintaining backwards compatibility is definitely the way to go and we can
> conclude that the new default semantics (especially per-connection vs.
> per-input buffers) have to be the same as in previous v2 releases. I hope
> you find it acceptable that a new version would certainly be not
> ABI-compatible and breaks some internal API calls that are typically not
> used directly by a user. This might break more advanced things like custom
> transport implementations. I fear that these kind of changes cannot be
> avoided completely, especially for the ConnFactory and ChannelElement
> classes.
>
> We should not limit the use cases of RTT on the core API level, like
> enforcing per-input storage objects for data connections and per-connection
> storage for buffer connections, but only add new use cases. It is more a
> question of having good documentation and choosing reasonable defaults that
> cover as many applications as possible, but at the end the application
> designer has to have the freedom to deploy the application with whatever
> connection semantics it demands for.
>
> In the following I try to wrap up and comment on some of the concerns that
> came up during the discussion so far, and suggest compromise solutions which
> have been at least partially discussed with Peter:
>
> 1) There is definitely a need for per-input port buffers AND for
> per-connection buffers - INDEPENDENT of whether the data/buffer object is
> installed on the reader and writer side.
>
> Indeed, according to the original proposal the questions of at what side of
> a remote connection the data object will be installed and whether there will
> be a per-input or a per-connection buffer cannot specified separately. Peter
> and I could not imagine why you would want separate buffers if the data is
> already available locally anyways (in the push case) and we still think that
> at least for data connections the current per-connection implementation can
> be considered as broken. Even with readNewest() there is no guarantee that
> you get the latest sample written. On the other hand, Sylvain correctly
> stated that there are no guarantees anyway as soon as remote connections (or
> any non-realtime transport) are considered and the only true solution is
> proper timestamping and synchronization. But I think we agree that for most
> deployments with only local connections the assumption that the latest
> sample is the last written sample holds and that timestamping is beyond the
> scope of RTT and can be left to user code or more advanced tools like the
> Rock stream aligner.
>
> Even with the proposed changes (let's call them v3 semantics as a working
> title) the connection policy is still primarily describing the connection,
> even if *some* settings limit the set of allowed policies for other
> connections, making *some* other settings de-facto a per-port setting. This
> would be primarily true for input ports and not at all for output ports,
> unless it has a connection of the new shared type. But it is not an option
> to associate the connection policy *only* with a port because not *all*
> possible connections imply per-port buffers. Specifying a connection policy
> on a writer/reader pair base as it is now and let certain invalid connection
> attempts fail is still the most general approach.
>
> Proposal: In order to not break existing applications, we introduce a new
> ConnPolicy field "read_policy" that toggles between per-input and
> per-connection buffers, instead of implying that by the existing "pull" flag
> for remote connections. The read_policy would be an enum with two possible
> values for now, "SingleInputBuffer" and "PreferCurrentConnection". The
> reason why we propose an enum here is that you could also think of other
> policies on how to select the next channel to read from in the
> per-connection case, e.g. RoundRobin, PreferLastSignaled, etc. The current
> v2 default would correspond to PreferCurrentConnection, as the channel that
> was last read with new data is preferred by the input port and only then it
> polls all other connections in the order they have been made. We have not
> thought this idea to the end yet and the names of the constants are still
> under discussion. We do not want to call the input buffers "shared", because
> the read policy is independent of the new concept of a "shared connection"
> where multiple input ports read from the same buffer. Or someone can come up
> with a better name for this type of connection...
>
>
> 2) Should the new per-input port buffers become the new default or not
> (SingleInputBuffer read policy)?
>
> With that respect the conclusion is that backwards compatibility is more
> important than the new "features" and the default parameters for the current
> ConnPolicy class should not be touched. However, there are different ways to
> tackle this:
>
> 2.1 Make it a compile-time option.
>
> 2.2 Only add *new* fields to the ConnPolicy class without changing the
> default parameter values in the constructor and static methods, and a new
> class ConnPolicy2 or ConnPolicy3 that inherits from ConnPolicy and might
> have other default parameters or another C++ API (I am thinking of a
> named-parameter based API) and can also be used for scripting. Internally,
> the connection factory would work with const references to the base class
> only.
>
> 2.3 We add a DefaultConnPolicy attribute to the GlobalsRepository. This
> application-wide (per process) policy instance will be copied to newly
> constructed ConnPolicy instances and the constructors and static methods
> only overwrite the respective parameters.
>
> It might be obsoleted by the above proposal, but in the meantime I added a
> compile-time option RTT_V2_COMPATIBILITY_MODE to my working branch (see
> ef99f2b), that, if set, reverts to the current v2 per-connection model, but
> lets RTT print meaningful warning messages in the case a new connection
> would be invalid with v3 semantics because of conflicts with other
> connections to the same port. Such a flag could smooth the transition phase
> and help application designers to find the minimal set of connections that
> require connection policy changes.
>
> There is one small exception from the backwards-compatibility rule, that I
> think is worth discussing:  The static method
>
> static ConnPolicy data(int lock_policy = LOCK_FREE, bool init_connection =
> true, bool pull = false);
>
> initializes the init_connection field to true by default, which was kind of
> surprising me, because it means that every new connection to an output port
> with this policy will overwrite a sample previously written by another port.
> Is this what we really want as a default? It is also not consistent with the
> constructor called as
>
> ConnPolicy(DATA)
>
> which would initialize the init_connection flag to false. The static method
> data(...) is only used in the OutputPortInterface::createDataConnection(...)
> method from within RTT and OCL itself.
>
> By the way, the init_connection flag is a good example where the API offers
> some degree of freedom to the application builder that semantically does not
> make sense at all for certain combinations, like in combination with a
> buffer connection. It is still not forbidden to set the flag, but it might
> be worth a warning log message if a connection is created like this.
>
>
> 3) How to add the "shared" concept within the current API, or to use a "new"
> API?
>
> This point was brought up by Janosch, who asked for "a more explicit
> interface for shared connections", with the main argument that it is
> fundamentally different from a normal connection and a "connect port A to B
> with policy C" kind of API does not fit well to that concept. That's true.
> We considered to use the createStream(...) API first, with a name_id string
> that identifies the shared buffer to connect to. This would actually fit
> better to the shared connection model, but it still enforces the same buffer
> type policies for all connections. We withdraw that idea because the
> existing API is more general and disallowing certain policy combinations is
> required anyway for private push connections with a single input buffer. At
> least in this case it cannot break existing applications. I do not think
> that there is a need for a completely new API only for shared connections.
>
> My current implementation in the updated-dataflow-semantics branch is as
> follows, and new/other ideas are always welcome:
>
> - Shared connection objects (the shared data object or buffered) are added
> to a process-wide registry. Every shared connection is identified by a
> unique string, either set explictly in the name_id field of the ConnPolicy
> or assigned automatically, in which case the name of the connection is
> returned in exactly this field (name_id is and always was declared as
> mutable).
> - For new connections with the name_id set, the shared connection is looked
> up in the registry, or created as new. As a consequence, if the same
> ConnPolicy instance is used to make a second connection, even if name_id was
> not set explicitly before, you will always connect to the same connection
> object.
> - For new connections with no name_id set, the port objects provide an API
> to return a pointer to an existing shared connection if they are already
> connected to an existing one, and only the non-connected port will be added.
> So ports can be connected in any order to each other, as long as there is
> always one of the two ports already connected. Otherwise the name_id has to
> be set or a new connection instance would be created. Perhaps this one could
> be dropped because it it too implicit and does not work very well for remote
> connections...
> - If an existing connection was found, the buffer policies (type, size and
> locking policy) have to match.
> - All ports will only store a single pointer to the shared connection
> instance in their connection list (in internal::ConnectionManager) and
> cannot know directly which other ports are connected.
> - For remote input ports with a shared connection, only a proxy is created
> locally and can be looked up in the shared connection repository. Other
> ports will only connect to either the  proxy or the real shared connection
> instance, but never build a new remote channel once the proxy has been
> created for the first remote connection (works in both directions).
> - All connect and disconnect calls to shared connections are logged with log
> level Debug to the RTT logger, so at least there is a manual way to check
> whether the resulting connections have been made as expected. Graphical
> tools like the rtt_dot_service could be patched later to visualize shared
> connections correctly.
>
>
> 4) Whether to encode the result of a write() in existing enums or not?
>
> Janosch asked for introducing a separate enum for the return value of
> write(...) calls, in order to not break or trigger compiler warnings for
> existing code that already uses the FlowStatus enum in a switch statement.
> Another minor issue with a single enum is the default value of FlowStatus
> attributes, which can be only meaningful for one use case.
>
> That's also fine for me and I will update the document and implementation
> accordingly (add a new enum WriteStatus). Note that the evaluation in
> boolean context is counter-intuitive in this case, other than for
> FlowStatus, because the value WriteSuccess (=0) value evaluates to false and
> all error cases (WriteFailure and NotConnected) evaluate to true.
>
>
> I hope I did not forget anything. This mail already became much longer than
> expected. I am sorry.
>
> I also did some performance measurements of read and write calls last week
> in terms of absolute time, CPU time, number of assignments and so on of the
> current, the new and even the v1 data flow implementations in different
> scenarios. I plan to come up with some results during this week. The tests
> revealed some bugs and performance caveats, and some of them are already
> present since the early days of RTT v2, e.g. that data samples might be lost
> (never read as NewData) because the read flag is not lock-protected or
> stored within the lock-free data structure. I am preparing a pull request to
> the current master branch, but it will also break the API of class
> base::DataObjectInterface.
>
> Best regards,
> Johannes
>
>
>
> On Mon, Sep 28, 2015 at 3:36 PM, S Roderick <[email protected]> wrote:
>>
>>
>> Johannes' proposed changes are focussed on
>> a) push vs pull, or specifying whether the data/buffer object is on the
>> reader or writer side. This is very useful for system designers dealing with
>> significant time delay.
>> b) private vs shared, whether a connection is per input/output port pair
>> or whether the connection is shared between multiple input/output ports.
>> c) mandatory, primarily intended to support knowing whether writes to a
>> buffer succeeded (so that traditional producer/consumer relationships have
>> some knowledge about the buffer state)
>>
>> It appears to me that most people are worried about changing the defaults,
>> more than they're worried about the actual proposed changes. Actual
>> technical concerns with the proposed changes appear to be
>>
>> 1) whether to encode the result of a write() in existing enums or not
>> 2) how to add the "shared" concept within the current API, or to use a
>> "new" API
>> 3) changing the RTT defaults, which then affect an application's
>> semantics.
>>
>> Did I miss any technical issues?
>>
>>
>> My huge personal caveat to all of this is that I do *not* think we should
>> change the current system semantics in introducing this change. Too many
>> Orocos changes have broken backwards compatibility and forced system
>> designers and implementors to change their system in response. I don't think
>> we should force that on users without an incredibly good reason. Otherwise
>> it breeds frustration in the community.
>>
>> If there is some way to introduce the proposed changes to the system, but
>> to not change the behavior of current systems, then I think that is a win
>> for all. There does seem to be some general agreement that there are valid
>> use cases that the current port implementation handles poorly, and that this
>> proposed approach does fix some of those. The proposed approach also
>> improves the situation for systems that don't want or need to use the flow
>> status approach. Both are equally valid approaches - IMHO it would be
>> beneficial to the community as a whole if RTT supported both approaches.
>>
>> Cheers
>> Stephen
>>
>>
>>
>> --
>> Orocos-Dev mailing list
>> [email protected]
>> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev
>>
>
-- 
Orocos-Dev mailing list
[email protected]
http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev