I'm reviewing the code on rock master, per merge request #10:
http://gitorious.org/orocos-toolchain/rtt/merge_requests/10
(note: gitorious screws up merge request numbering, since I already merged
once a merge request #10 in november last year...)
I'd like to have more information on commits
2ece028
types: add support for marshallers to have internal data structures (cookies)
and
89bd9f5
mqueue: make it zero-copy on write and single-copy on read, fix handling of
data_sample
2ece028: Is the cookie necessary to give the marshaller a chance to do some
non-real-time work on beforehand, for each channel that needs marshalling ?
Ie, is the sharing of the transport object between all data channels the
reason why a cookie is needed ?
Adding a void* cookie = 0 to a virtual function is only helpful for the client
code, not for the implementors of type marshallers.It gives a false feeling of
backwards compatibility :-). Given the number of transports out there (and the
people doing them equally small), I think this is an acceptable pain, also
given the necessity of the patch. Some up-front discussion would have been
appreciated though.
89bd9f5: The commit message says:
"This changes the MQ send/receive logic to remove the intermediate
ValueDataSource when possible (i.e. on the write path), which makes
write zero-copy in principle -- there is still, for an unknown reason,
a buffer between the output port and the MQ. The read path has one copy,
but is now made RT-friendly by removing a temporary (and therefore, one
copy).
Moreover, the data_sample vs. write code paths are now merged. The logic
that was assuming that at least one write/data_sample would come during
connection is removed, making the handling of data_sample on par with
the one in other transports (i.e. one can use MQ without an initial
data sample at all)."
Is this comment correct ? I still see a data_sample() implementation in
mqueue, and a check for it in inputReady(), being similar/identical to the
situation before..? So which point is being made here ?
Other notes:
In TypeTransporter, I believe createStream needs to return a shared_ptr
instead of a plain *, since the refcount could drop to zero and remove the
channel element object before we have a chance to return it. I've seen Visual
Studio smart-assing this situation with memory corruption as a consequence...
while exactly the same code works fine under GCC.
All-in-all, I'd like to merge this asap such that it is ready for 2.4.0. It
caused as-good-as-no merge conflicts. Merge requests should reach me faster
now, since gitorious finally allows me to set the default email option to 'on'.
Peter
rock-master : cookie versus blob in transport
> 2ece028: Is the cookie necessary to give the marshaller a chance to do some
> non-real-time work on beforehand, for each channel that needs marshalling ?
> Ie, is the sharing of the transport object between all data channels the
> reason why a cookie is needed ?
Yes, it is the case. The other option would have been to force people to
clone the transport object on each channel, but I thought the cookie
solution would be less intrusive.
> Adding a void* cookie = 0 to a virtual function is only helpful for the client
> code, not for the implementors of type marshallers.It gives a false feeling of
> backwards compatibility :-).
I agree that, in principle, the "new" type marshallers that require
cookies can't be used in places where the user does not care about
cookies. Given that, in practice, each type marshaller is used in only
one place, I thought that it would be an acceptable middle ground (i.e.
minimal changes to existing code)
> Given the number of transports out there (and the
> people doing them equally small), I think this is an acceptable pain, also
> given the necessity of the patch.
> Some up-front discussion would have been
> appreciated though.
Agreed. Unfortunately, I had no time for that. CORBA was making our
system unusable and I had to make that stuff working weeks before I
started to implement it ...
> 89bd9f5: The commit message says:
>
> "This changes the MQ send/receive logic to remove the intermediate
> ValueDataSource when possible (i.e. on the write path), which makes
> write zero-copy in principle -- there is still, for an unknown reason,
> a buffer between the output port and the MQ. The read path has one copy,
> but is now made RT-friendly by removing a temporary (and therefore, one
> copy).
>
> Moreover, the data_sample vs. write code paths are now merged. The logic
> that was assuming that at least one write/data_sample would come during
> connection is removed, making the handling of data_sample on par with
> the one in other transports (i.e. one can use MQ without an initial
> data sample at all)."
>
> Is this comment correct ? I still see a data_sample() implementation in
> mqueue, and a check for it in inputReady(), being similar/identical to the
> situation before..? So which point is being made here ?
As far as I remember, I reverted that part of the commit later on. The
"remove copies" is valid, though.
rock-master : cookie versus blob in transport
On Thursday 17 March 2011 07:17:00 Sylvain Joyeux wrote:
> > 2ece028: Is the cookie necessary to give the marshaller a chance to do
> > some non-real-time work on beforehand, for each channel that needs
> > marshalling ? Ie, is the sharing of the transport object between all
> > data channels the reason why a cookie is needed ?
>
> Yes, it is the case. The other option would have been to force people to
> clone the transport object on each channel, but I thought the cookie
> solution would be less intrusive.
ok.
>
> > Adding a void* cookie = 0 to a virtual function is only helpful for the
> > client code, not for the implementors of type marshallers.It gives a
> > false feeling of backwards compatibility :-).
>
> I agree that, in principle, the "new" type marshallers that require
> cookies can't be used in places where the user does not care about
> cookies. Given that, in practice, each type marshaller is used in only
> one place, I thought that it would be an acceptable middle ground (i.e.
> minimal changes to existing code)
>
> > Given the number of transports out there (and the
> > people doing them equally small), I think this is an acceptable pain,
> > also given the necessity of the patch.
> >
> > Some up-front discussion would have been
> > appreciated though.
>
> Agreed. Unfortunately, I had no time for that. CORBA was making our
> system unusable and I had to make that stuff working weeks before I
> started to implement it ...
well, no harm done I guess.
>
> > 89bd9f5: The commit message says:
> >
> > "This changes the MQ send/receive logic to remove the intermediate
> > ValueDataSource when possible (i.e. on the write path), which makes
> > write zero-copy in principle -- there is still, for an unknown reason,
> > a buffer between the output port and the MQ. The read path has one copy,
> > but is now made RT-friendly by removing a temporary (and therefore, one
> > copy).
> >
> > Moreover, the data_sample vs. write code paths are now merged. The logic
> > that was assuming that at least one write/data_sample would come during
> > connection is removed, making the handling of data_sample on par with
> > the one in other transports (i.e. one can use MQ without an initial
> > data sample at all)."
> >
> > Is this comment correct ? I still see a data_sample() implementation in
> > mqueue, and a check for it in inputReady(), being similar/identical to
> > the situation before..? So which point is being made here ?
>
> As far as I remember, I reverted that part of the commit later on. The
> "remove copies" is valid, though.
Ack. I see no immediate reasons reject the changes then. I'll go over it
again and then merge it to master.
Peter