initial support for pmr containers in DPL by mkrzewic · Pull Request #1593 · AliceO2Group/AliceO2

mkrzewic · 2019-01-15T11:05:17Z

depends on #1581

ktf

Please do not change:

class FairMQDevice;

to

#include <FairMQDevice.h>

the rest I am not sure I like, but since I've an idea on how to completely hide DataAllocator all this from the user, I can live with it.

mkrzewic · 2019-01-15T16:46:53Z

@ktf, indeed, the need for include was already gone, forgot to change that back. done.

mkrzewic · 2019-01-15T16:48:20Z

@ktf on another note - according to @rbx the boost includes are gone - is that verified?

ktf · 2019-03-18T10:51:24Z

I had a bit of time to rethink about this. I think you have been too conservative. ;-) IMHO, what we should do is to replace newChunk and adoptChunk to basically be what you do in makeVector / adoptVector, basically having std::pmr::vector replace the DataChunk. We can then discuss how to expose a proper API for all the other types, but given that FairMQ now basically supports resizable messages I do not see why DataChunk should limit us to fixed size... I would still not expose the container to the user, like in the current interface, but I guess that for the backend there is not much to gain from having it based around fixed size messages no?

@matthiasrichter what do you think?

mkrzewic · 2019-03-19T09:28:48Z

AFAIK there is no support for resizeable messages in FairMQ - last I checked you could only specify a part of the allocated buffer to be sent, the full resize functionality lies with the container (@rbx something new was added?).
Anyway the point of this PR is exactly to expose a container and skip the "ownership manager" abstraction layer, there is no need for that in modern C++, it just makes things complicated.
I consider the "adoptContainer" vital functionality. The "makeVector" part is just for convenience introduced at @ktf request, not really necessary.
I think this PR does not break anything in DPL, so I don't see why it takes so long to merge.

davidrohr · 2019-07-10T09:02:01Z

@ktf @mkrzewic : This has been stalled since >3 months now. Do we still need it, then we should rebase and merge it at some point?

ktf · 2019-07-10T09:15:09Z

I would like to see the developments of @matthiasrichter in the same direction before deciding what to do with this.

matthiasrichter · 2019-07-11T16:29:16Z

I think its independent of the other development which is basically an extension of #1873 / #1846. Its a matter of two different approaches and the arguments have been posted here in this PR.

davidrohr · 2019-07-11T16:42:13Z

Well, if it is independent and provides useful functionality, I don't see a reason not to merge it, in particular since it is a rather small change and looks elegant. But to be honest, I didn't dive deep enough into the framework to judge if this is just a competing strategy to #1846.

sawenzel · 2019-09-09T06:39:18Z

Is there any new development here? Probably a decision needs to be taken at some point.

mkrzewic · 2019-09-17T12:05:57Z

Dear all, i updated the PR to use Matthias' new MessageContext thingy thereby improving compliance with the way-DPL-does-it. As a reminder: this only adds functionality without breaking or changing anything (except for fixing the broken DPLMerger which looks like dead code anyway).

davidrohr

Looks good to me (besides the clanf-format)

davidrohr · 2019-09-17T12:07:56Z

Framework/Utils/include/DPLUtils/Utils.h

 namespace workflows
 {
-//TODO: this is to make DPLmerger compile, but this code is (and always was) horribly broken - please remove.
-inline void freefn(void* data, void* /*hint*/) { delete static_cast<char*>(data); };


Difficult to judge for me, but if this is broken and should be removed, and it compiles after removal, then it is probably the way to go. I mean, why is it there anyway? Was it just forgotten to remove it?

probably, a remnant after removal of the Stack::freefn long time ago.

davidrohr · 2019-09-17T16:48:01Z

@ktf @sawenzel : Could you have a look?

ktf · 2019-09-17T20:40:51Z

Ok, fine with me at this point. I still disagree on the idea, but I guess we need something. @matthiasrichter what do you think?

sawenzel

Fine with me. @mkrzewic Could you maybe target to add a short markdown documentation how the feature could be used.

ktf · 2019-09-18T09:56:44Z

Yes, indeed, documentation and a test would be nice. I assume there is no way to simply call the two adopt and make?

ktf · 2019-09-18T10:02:32Z

Actually makeVector could become make<o2::vector<T>> and be folded as one of the constexpr ifs, no?

mkrzewic · 2019-09-18T10:07:05Z

@ktf, indeed, looks like it could. About the test: I'll think about a test workflow, not sure how to test this "standalone" to actually trigger all the involved dpl internals...

ktf · 2019-09-18T10:53:34Z

@mkrzewic there is a test already for DataAllocator:

https://github.com/AliceO2Group/AliceO2/blob/dev/Framework/Core/test/test_DataAllocator.cxx

Something simple there would do.

For the renaming we can do it afterwards, unless you find it completely trivial to do.

mkrzewic mentioned this pull request Jan 15, 2019

Stack fixes #1581

Merged

ktf self-requested a review January 15, 2019 15:56

ktf requested changes Jan 15, 2019

View reviewed changes

mkrzewic force-pushed the pmr_in_dpl branch from 55cba32 to 35d6690 Compare January 16, 2019 11:39

mkrzewic force-pushed the pmr_in_dpl branch from 35d6690 to 0484444 Compare January 31, 2019 13:07

ktf mentioned this pull request Mar 15, 2019

DPL: align FairMQDeviceProxy to FairMQDevice::GetChannel defaults #1804

Merged

Initial direct support for pmr aware containers in DPL

17cc067

mkrzewic force-pushed the pmr_in_dpl branch from 0484444 to 17cc067 Compare September 17, 2019 12:01

mkrzewic requested a review from a team as a code owner September 17, 2019 12:01

davidrohr approved these changes Sep 17, 2019

View reviewed changes

clang-format

6ab1557

mkrzewic force-pushed the pmr_in_dpl branch from 912eb37 to 6ab1557 Compare September 17, 2019 12:36

sawenzel approved these changes Sep 18, 2019

View reviewed changes

ktf requested a review from matthiasrichter September 18, 2019 09:56

ktf merged commit b6848e3 into AliceO2Group:dev Sep 21, 2019

knopers8 pushed a commit to knopers8/AliceO2 that referenced this pull request Oct 23, 2019

DPL: Initial support for pmr containers (AliceO2Group#1593)

b6e8a5f

carlos-soncco pushed a commit to carlos-soncco/AliceO2 that referenced this pull request Oct 28, 2019

DPL: Initial support for pmr containers (AliceO2Group#1593)

62b93fd

Conversation

mkrzewic commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktf left a comment

Choose a reason for hiding this comment

Uh oh!

mkrzewic commented Jan 15, 2019

Uh oh!

mkrzewic commented Jan 15, 2019

Uh oh!

ktf commented Mar 18, 2019

Uh oh!

mkrzewic commented Mar 19, 2019

Uh oh!

davidrohr commented Jul 10, 2019

Uh oh!

ktf commented Jul 10, 2019

Uh oh!

matthiasrichter commented Jul 11, 2019

Uh oh!

davidrohr commented Jul 11, 2019

Uh oh!

sawenzel commented Sep 9, 2019

Uh oh!

mkrzewic commented Sep 17, 2019

Uh oh!

davidrohr left a comment

Choose a reason for hiding this comment

Uh oh!

davidrohr Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkrzewic Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

davidrohr commented Sep 17, 2019

Uh oh!

ktf commented Sep 17, 2019

Uh oh!

sawenzel left a comment

Choose a reason for hiding this comment

Uh oh!

ktf commented Sep 18, 2019

Uh oh!

ktf commented Sep 18, 2019

Uh oh!

mkrzewic commented Sep 18, 2019

Uh oh!

ktf commented Sep 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

mkrzewic commented Jan 15, 2019 •

edited

Loading

davidrohr Sep 17, 2019 •

edited

Loading