Adopt FairMQMessage backed memory resource collection from AliceO2 #93
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #93 +/- ##
==========================================
+ Coverage 71.43% 71.49% +0.05%
==========================================
Files 69 71 +2
Lines 4071 4076 +5
==========================================
+ Hits 2908 2914 +6
+ Misses 1163 1162 -1
|
|
the ci errors seem unrelated? it fails some unrelated test it looks like |
|
Hm, ya, sry, I also wanted to come back to this anyways. But this week, no chance, will have a look next week. Sry. |
|
Some thoughts on the APIs: Currently, we have on sender side: auto pmr = channel.Transport()->GetMemoryResource();
std::vector<MyData, std::pmr::polymorphic_allocator<MyData>> v(
std::pmr::polymorphic_allocator<MyData>{pmr});
auto message = fair::mq::getMessage(std::move(&v[0]));
channel.Send(message);here, I would like to present something like this to the user: auto v = fair::mq::MakeVectorFor(channel);
channel.Send(std::move(v));For receiving side, we could have just something like this: template<typename T> gsl::span<T> FairMQMessage::GetSpan();Not sure about adopting buffers with STL containers. Do you know, if this is "legal" usage? What are your ideas how the API should look like? |
|
Hi @dennisklein, as for your question about whether the standard allows buffer adoption - it does not forbid it:) See MemoryResourceTools.h for some ideas on how to do this for vector (the adoptVector family of functions, maybe should be called adoptBuffer or sth) - (zero-copy) adoption of buffers in a std::vector (or any other container) is in principle possible, albeit not with the standard allocators. In principle there is no reason to create a std::vector and receive a span, we can present a vector. This is a topic for a longer discussion i guess which we can open any time:) |
I am still not sure about buffer adoption with e.g. |
|
@dennisklein I don't think so, construct/destroy is part of the public interface of allocator_traits and the user can do whatever (in principle), I could not find any "legal" problem with this approach - it is perfectly legal to operate on unitialized memory, standard allows that explicitly afaik. Now doing this might produce unexpected behaviour if the buffer was produced on a machine with a different layout/endianness etc. so the user must make sure this is OK of course, but that is up to a higher level layer (user or lib). anyway this kind of implementation can always be made in the user layer, fairmq does not have to implement/expose it directly. |
E.g. https://wg21.link/containers#container.requirements.general-3 states "objects stored in these components shall be constructed using the function |
|
@mattkretz well, so is (in general) using reinterpret_cast - or span - which essentially all boil down to the same thing. In this case we are not talking about a general case but about vectors of numbers: contiguous sequences of simple elements, simple as in construction/destruction has no side effects, data is standard layout... So we either do the proper thing and copy the data from buffer to container loosing oodles of time and resources or we reinterpret cast (or use span, which is just reinterpret_cast with some sugar coating). I don't see how this is UB - I'm not constructing before construction - I'm just defining a special construction strategy. Is it cheating? Perhaps:) |
|
You can certainly use AFAIU your specific hack, I believe you may get away with it - as long as users don't start using the full The reason why I believe this hack isn't worth the trouble, is that you need a different type than |
|
:) sure, I wasn't actually pushing for this specific hack to make it in the library (at this level ;) ), it is not the reason for this PR. |
|
oh, btw, we do need a type different than just |
|
Since the |
fairmq/MemoryResourceTools.h
Outdated
| size_t nelem, | ||
| SpectatorMessageResource *resource) | ||
| { | ||
| return std::vector<const ElemT, SpectatorAllocator<const ElemT>>( |
There was a problem hiding this comment.
Why is the conversion from std::vector<const ElemT, SpectatorAllocator<const ElemT>> to std::vector<const ElemT, boost::container::pmr::polymorphic_allocator<const ElemT>> possible? At least https://en.cppreference.com/w/cpp/container/vector/vector doesn't list any ctor that would allow it.
There was a problem hiding this comment.
that is because SpectatorAllocator derives from polymorphic_allocator i guess...
There was a problem hiding this comment.
That doesn't help. std::vector still doesn't have a ctor that takes a std::vector with different allocator type: https://godbolt.org/z/1bGCWi
There was a problem hiding this comment.
hmm, somehow here it works... actually, in the original implementation in O2 the return type is auto, I had to invent something here for c++11. It is removed from this PR anyway, it lives on in #110 - but it's days are probably numbered as it is a bit hard to use - the owning version is better (or less bad ;) )
There was a problem hiding this comment.
ah i think I know why it worked here: it never did - the template is never instantiated as the function is never used :)
There was a problem hiding this comment.
:-) I suspected as much. Normally, going from one allocator type to the next implies a reallocating and moving the data. That's why I believe this conversion ctor is not part of the std::vector interface. ...and why polymorphic_allocator was needed so badly.
Not yet. But I triage this PR to the 1.4 milestone, since I want to make a stable 1.3.x release this week, and hurrying here does not help anyone. (This will also delay the Also, can you please rebase on current dev, I hope this will make the CI checks green. |
|
fine, unless someone in O2 would like to see the core functionality already in 1.3 - @ktf ? I would possibly like to make one more change: we moved O2 to C++17 and we are cleaning up some "hacks" like we would e.g. like to start using std::byte . I defined fair::mq::byte to alias unsigned char, it would be nice to maybe make the definition dependent on the language standard used to compile fairmq with? Would that be possible/doable? |
Also fine with me, from my point we can merge the
There is a good chance FairMQ will also move to C++17 with 1.4. We are currently testing. Will know more in one or two weeks. |
Add a pmr interface to FairMQTransportFactory refactor Port the unit tests for MemoryResources clang format
the patch seems big but most of it is just propagating the new notion of constness of the factory - since it keeps track of created messages with the internal allocator it no longer is const
|
for now I removed the adoption related bits then. Having some problems building it locally after rebase, let's see what the ci says. local build and tests succesful |
so the eventual switch to std::pmr becomes easier
dennisklein
changed the title
|
The failing test on Linux is probably unrelated to your changes, we are looking at it. Apple Clang finds this: https://cdash.gsi.de/viewBuildError.php?type=1&buildid=190840 |
|
@dennisklein not sure if changing this won't break C++11 builds - copy elision is only required from C++17 on, returning a move only thing by value might break the build on a compiler that does not have this as an optimisation for c++11... difficult to test on my machine, since I always compile with -std=c++17 :) |
The warning means that
You can pass |
|
Fine with me if we need to move to some new tag.. |
As discussed in #54 opening a PR for review. This contains the port of the O2 pmr prototype + basic tools + basic integration into FairMQ + some unit tests (essentially the same as in O2).
Next step in the short term could be additional integration steps (e.g. adding a pointer to factory in FairMQMessage) followed by some simplification of the tools.
In the longer term we will see...