Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote vs local transparency difference when reading spectrum or image attributes #416

Open
bourtemb opened this issue Dec 4, 2017 · 2 comments · May be fixed by #697
Open

Remote vs local transparency difference when reading spectrum or image attributes #416

bourtemb opened this issue Dec 4, 2017 · 2 comments · May be fixed by #697

Comments

@bourtemb
Copy link
Member

@bourtemb bourtemb commented Dec 4, 2017

When a Tango client tries to read a spectrum or image attribute which is not polled from a device exported by the same device server process as the client, by default (when Attribute::set_value() release parameter is set to false), omniORB and Tango will not do any copy of the data buffer and will give the spectrum/image buffer data pointer to the client without transferring ownership of this buffer to the client.
In this case, if the client modifies directly the data pointed by the pointer it got, it will also modify the data on the server side's buffer (dangerous!!).
In the case of reading a spectrum or image polled attribute, Tango is already making a copy of the data and providing the copy/transferring ownership to the client. So in this case, the client can do whatever it wants with the data pointer it got. It is the same in the case of a remote client.

Here are some parts from the answer we got from omniORB's maintainer on this topic:

The C++ sequence mapping intentionally breaks local/remote transparency with regard to the release flag, for efficiency purposes.
[...]
Really, the summary is that the ability to construct a sequence with release set to false is a short-cut that can help performance, at the expense of a loss of local/remote transparency.

If it is essential that client code does not need to care about this, then yes, the implementation should avoid using sequences with release set to false. That will come with a bit of a performance cost, which may or may not be a problem for your application.

The alternative is for clients to become aware of this situation. If a client wants to modify the data in a sequence it has received, it can look at its release flag. If release is false, it knows the sequence is "borrowing" the buffer, so it should create a new sequence object with the copy constructor, because that copies the buffer as well.

This issue was created in order to get always the same behaviour on the client side, meaning, the client can do whatever it wants with the data it got after reading a spectrum or image attribute. No matter whether the read was done on a local (same process) or remote device.
In order to avoid performance penalties, the best would be that Tango would detect whether the client is local or remote and if the client is local, Tango would always force a copy. In the remote call case, we should stick with the current behaviour and avoid an unnecessary copy.

The code creating the result sequences is located in DATA_IN_OBJECT(A,B,C,D) macro in server/device.h file.

@bourtemb bourtemb added the bug label Dec 4, 2017
@Ingvord Ingvord added this to the Q1'2018 milestone Feb 5, 2018
@mliszcz

This comment has been minimized.

Copy link
Collaborator

@mliszcz mliszcz commented Dec 19, 2019

Hi @bourtemb

I'd like to work on this issue.

We have two possible places to copy the sequence:

  1. At server side in DeviceImpl::data_into_net_object, detect local client and copy the sequence (the_seq = *ptr) instead of calling replace:
    if (aid.data_5 != Tango_nullptr) \
    { \
    (*aid.data_5)[index].value.D(C); \
    A &the_seq = (*aid.data_5)[index].value.D(); \
    the_seq.replace(ptr->length(),ptr->length(),ptr->get_buffer(),ptr->release()); \
    if (ptr->release() == true) \
    ptr->get_buffer(true); \
    } \

    I propose to detect local client basing on ClntIdent, but will work only for IDL 5 and 4 devices. Also it will require to somehow pass client pid to data_into_net_object.
  2. At client side, in ApiUtil::attr_to_device_base, check for release flag and copy the sequence (dev_attr->DoubleSeq = new DevVarDoubleArray(tmp_seq)) instead of creating new sequence with the same data buffer:
    else
    {
    tmp_db = const_cast<CORBA::Double *>(tmp_seq.get_buffer());
    dev_attr->DoubleSeq = new DevVarDoubleArray(max,len,tmp_db,false);
    }

    This solution will allow to by-pass the new behavior by invoking CORBA method read_attributes_5 directly. Depends how you look at this, it may be pro or con.
@mliszcz mliszcz self-assigned this Dec 19, 2019
@mliszcz mliszcz moved this from To Do to In Progress in Tango 9 Long Term Support Dec 19, 2019
@mliszcz mliszcz removed their assignment Feb 3, 2020
@mliszcz mliszcz added the question label Feb 3, 2020
@mliszcz

This comment has been minimized.

Copy link
Collaborator

@mliszcz mliszcz commented Feb 3, 2020

As discussed on last Tango Kernel meeting, I'm adding "Question" label. In previous comment I listed two possible solutions. I'm in favor of the second one (doing detection at client side), but I'd like to hear opinions from other developers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Tango 9 Long Term Support
  
To Be Reviewed
3 participants
You can’t perform that action at this time.