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

Support min alignment in ChannelResource #282

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

dennisklein
Copy link
Member

@dennisklein dennisklein commented Jun 17, 2020

Related to #265

@dennisklein dennisklein requested a review from mkrzewic June 17, 2020 12:45
@dennisklein dennisklein linked an issue Jun 17, 2020 that may be closed by this pull request
Copy link
Contributor

@mkrzewic mkrzewic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is MinAlignment set? is it something that can be set "globally"? (say once per factory or so?)

/// This is the allocator that interfaces to FairMQ memory management. All
/// allocations are
/// delegated to FairMQ so standard (e.g. STL) containers can construct their
/// stuff in
/// memory regions appropriate for the data channel configuration.
class ChannelResource : public FairMQMemoryResource
template<std::size_t MinAlignment>
class AlignedChannelResource : public FairMQMemoryResource
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the name change necessary? Since the default behavior does not change (or does it?), signatures do not change (as far as I can see) it seems to be the same animal, just a better behaved one :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I don't change the name, it would not let me do the name alias: using ChannelResource = ChannelResource<0>; - do you know how to make it work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, good question :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

template <int alignment = 0>
struct ChannelResource {
};

no?

@dennisklein
Copy link
Member Author

How is MinAlignment set? is it something that can be set "globally"? (say once per factory or so?)

It would be per factory, but one could create more than one per factory as before, see usage in the unit test in this PR. The TransportFactory::GetMemoryResource() is unchanged.

@mkrzewic
Copy link
Contributor

ok, so it looks essentially like the workaround we have in O2 (@ktf ?). This would give us no further advantage aside from not having this specific allocator in our code base. How about the thing we briefly discussed with @rbx (AliceO2Group/AliceO2#3674 (comment)) for a bit more transparency?

@dennisklein
Copy link
Member Author

ok, so it looks essentially like the workaround we have in O2 (@ktf ?). This would give us no further advantage aside from not having this specific allocator in our code base.

Yes, it is nothing more.

How about the thing we briefly discussed with @rbx (AliceO2Group/AliceO2#3674 (comment)) for a bit more transparency?

Sure, I am fine with making the default alignment configurable on that API layer, too.

Just the same question for my understanding, whats the improvement over just using the pmr interface above to create messages? Or, in other words, what makes the pmr solution - even if only a specialized pmr class in O2 repo - just a workaorund? Are we just talking about API design or is there any technical difference you want to see?

@mkrzewic
Copy link
Contributor

@dennisklein I'd say it is about API design mostly, technically we have all the pieces, but just would like to avoid confusion. TransportFactory::GetMemoryResource() (and the implicit conversion operator) are just too seductive not to use :) Just saying it could be potentially dangerous if the default API (above) and the thing we actually need (explicitly constructed allocator as it is now) return the same thing different only in subtle ways like (over-)alignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants