ComputationalRadiationPhysics / graybat

Graph Approach for Highly Generic Communication Schemes Based on Adaptive Topologies :satellite:
Other
8 stars 4 forks source link

Sending/receiving dynamic types #121

Closed fabian-jung closed 7 years ago

fabian-jung commented 7 years ago

It is possible to send and receive dynamic types like std::vector at the moment, but the receiver has to allocate the memory before receiving the data. I think this is very unconvenient behaviour and how to make it more intuitive. In addition to this it would be nice to add support for more types. This may also solve Issue #67.

One example for a hard to find error at the moment is this:


Sender:
std::vector<int> numbersToSend { 1, 2, 3 }
cage.send(numbersToSend, edge);

Receiver:
std::vector<int> numbersToReceive;
cage.recv(numbersToReceive, edge);
print(numbers); // will print an empty vector

I think an elegant solution would be to customize the send and receive with type traits, that are implemented in graybat or by the user. The implementation for std::vector could use the vector::data() and vector::size() mechanic on the sender side and a vector::resize() before memcpying on the receiver side.

erikzenker commented 7 years ago

Is there a problem to allocate memory in advance ? From the example I don't see the problem to construct the receive vector with 3 elements:

std::vector<int> numbersToReceive(3);

If you do not know how much to receive in advance then send the number of elements first and then receive n elements. Maybe I misunderstand the use case for such a feature.

theZiz commented 7 years ago

I think, @fabian-jung 's problem is, that you still have to deal with the sizes yourself. It would be very handy to be able to "just" send a vector from a vertex to another without any need to check for enough size.

I think using type traits for send and recv would be a good solution.

Another idea to avoid such bugs is to send the size of the payload (in bytes) in debug mode additionally and to check with asserts in the recv method. Only problem would be, that applications compiled with and without debug mode would not be compatible anymore if we do not add new datatypes like DEBUG_INT, which tells the receiver, that the real payload starts after the size even if it is compiled in release mode.

erikzenker commented 7 years ago

Ok then let us discuss how this can look like.

fabian-jung commented 7 years ago

I thought the receiver would get the complete message anyway and just the memcpy is limited to the allocated space. I think the easiest way would be to write a type trait, that deals with the serialisation both in send and recv funktions. The traits option is also easy to extend for non stl types. The tradeoff is, that implemented traits are hard to overwrite and may come with runtimeoverhead. Altough i wouldn't mind the overhead if you get full stl support for that.

include/graybat/communicationPolicy/socket/base.hpp:

template <typename T_CommunicationPolicy>
template <typename T_Recv>
auto Base<T_CommunicationPolicy>::recvImpl(graybat::communicationPolicy::MsgType<T_CommunicationPolicy> const msgType,
    graybat::communicationPolicy::Context<T_CommunicationPolicy> const context,
    graybat::communicationPolicy::VAddr<T_CommunicationPolicy> const srcVAddr,
    graybat::communicationPolicy::Tag<T_CommunicationPolicy> const tag,
    T_Recv& recvData)
-> void {

    using Message   = graybat::communicationPolicy::socket::Message<T_CommunicationPolicy>;

    Message message(std::move(inBox.waitDequeue(msgType, context.getID(), srcVAddr, tag)));
    memcpy (static_cast<void*>(recvData.data()),
    static_cast<std::int8_t*>(message.getData()),
    sizeof(typename T_Recv::value_type) * recvData.size());

}

My draft:

template <typename T_CommunicationPolicy>
template <typename T_Recv>
auto Base<T_CommunicationPolicy>::recvImpl(graybat::communicationPolicy::MsgType<T_CommunicationPolicy> const msgType,
    graybat::communicationPolicy::Context<T_CommunicationPolicy> const context,
    graybat::communicationPolicy::VAddr<T_CommunicationPolicy> const srcVAddr,
    graybat::communicationPolicy::Tag<T_CommunicationPolicy> const tag,
    T_Recv& recvData)
-> void {

    using Message   = graybat::communicationPolicy::socket::Message<T_CommunicationPolicy>;

    Message message(std::move(inBox.waitDequeue(msgType, context.getID(), srcVAddr, tag)));
    MessageTrait<declType(recvData)>::unfoldMessage(recvData, message);

}

And vica versa for the send function.

erikzenker commented 7 years ago

I think we need to start at the cage or communicationPolicy interface level. Everything underneath this layer is too specialized. Since, you would have to write a specialization for your types for every communicationPolicy.

What do you think about the following sketch. It uses a probe function to get the size of incoming data:

recv(&resizeableContainer){
  auto status = comm.probe();
  resizeableContainer.resize(status.size());
  comm.recv(resizeableContainer);
}

But, for performance reasons I do not want to force a recv method to probe. Maybe we just probe on empty containers?

fabian-jung commented 7 years ago

I'd love to implement it at cage level. I also like the idea with the probe function, but we should not hard code the behaviour in the recv. There should be a way to specify different behaviour for different types. I also think, that there should not be multiple calls for resizing and non resizing receives, since this can lead to really hard to find errors. The receive should behave like the container is intended to be.

You are right, that the probe is may be unneccessary on some cases. The decision to probe or not is associated with the receive type.

std::vector -> probe, correctness, conveniance std::array -> dont probe, maximum performance

recv(&data){
  receiveTrait<decltype(data)>::receive(comm, data);
}

template <typename type>
class receiveTrait;

template<>
class receiveTrait<std::array> {
    void receive(...) {
        comm.recv(...);
    }
};

template<>
class receiveTrait<std::vector> {
    void receive(...) {
        auto status = comm.probe();
        resizeableContainer.resize(status.size());
        comm.recv(...);
    }
};
theZiz commented 7 years ago

I think, that type traits are the right way to go like @fabian-jung said. They are easy to extend for new types, we can predefine useful traits for STL types and the semantic behaviour stays the same. The only thing, which could change, is the run time behaviour. But tbh I doubt, that probing and resizing makes any noticeable runtime difference, but if even the new behaviour is slower then

The CommunicationsPolicy of course need to use these traits, which could be

template <typename Comm,typename Type>
Cage::recv(Comm &comm,Type& t)
{
  Cage::Traits::recv<Comm,Type>(comm,t);
}

template <typename Comm>
Cage::Traits::recv<std::vector>(Comm& comm, std::vector& v)
{
  //std::vector case with resize using comm.probe()
}

template <typename Comm>
Cage::Traits::recv<std::array>(Comm& comm, &std::array v)
{
  //std::array case without resize only using comm.recv()
}

This solutions works on the cage level and the communication policies "only" need to implement some kind of probing.

Edit: Editing comments is unfair. -_-

erikzenker commented 7 years ago

Okay, I think that this is a useful feature and will provide an implementation soon.

fabian-jung commented 7 years ago

I have had an idea for the "default" case. In #include <type_traits> there is an trait std::is_trivially_copyable which is true for all types that can be copied bitwise. (With the exception of raw pointers within the class) Together with SFINAE one could implement a implementation of the recv trait for all trivial types and containers. If later users try to send non trivial types, they would get a compiler error.

Edit: Fixed typos.

theZiz commented 7 years ago

woah

fabian-jung commented 7 years ago

I implemented the Trait in the last days. It does add support for all trivial types and those, that are built by those. (e.g. std::array with trivial type or std::pair) The code is currently in the cracen repo, but i will move it into graybat soon. I just have to figure out, how to integrate it without changing the api to the outside world.

erikzenker commented 7 years ago

This is even better (no work for me). I am looking forward to your pull request. Thx a lot.

fabian-jung commented 7 years ago

To make things shorter TCT means trivial copyable types.

For the moment my implementation support TCT, structs with TCT, std::vector of TCT and string and some more that come with the default case (std::array, std::pair and many more).

What is not yet implemented are the adapters for the transitiv hull of those types. E.g. std::vector<std::vector> and so on. It's a low hanging fruit, because it would be very easy to implement, but comes at the cost of copying the data together to one bitfield. In my eyes it seems to be the fastes way to serialise data if it is needed and could be a very interesting feature. What do you think of this approach?

erikzenker commented 7 years ago

I think that it should be one (maybe the default) approach to handle this kind of data. We need to investigate further how we can combine this issue with the serialization problem of #123.

But in the beginning it is good to have some implementation that we can talk about later and improve further.

fabian-jung commented 7 years ago

I opened a pull request to work with.

erikzenker commented 7 years ago

Probe was merged in #132