Add a customization point for put_parcel so we can override actions (… #2377
Conversation
| { | ||
| if (components::component_invalid == addr.type_) | ||
| { | ||
| addr.type_ = components::get_component_type< |
There was a problem hiding this comment.
The setting of component type should be done in the put_parcel functions already.
There was a problem hiding this comment.
Does this mean that this line can be removed?
There was a problem hiding this comment.
No, that means it should happen one layer above.
|
The specialization at this level was intended to bypas the generation of a 'parcel' altogether. I wanted to intercept things before put_parcel, but since I've not actually implemented the rdma actions yet, I'm open to these suggestions. I will play some more and start implementing it and see where it leads... |
|
While thinking about this a little more, we already have this specialization in place: It's the message_handler that can be overriden for a given kind of action (we already do this for parcel coalescing). This message handler could then perform the necessary rdma action given the specific parcel (the creation of a parcel mainly involves the construction of a transfer_action). |
The advantage of the proposed customization point is that it avoids to create a parcel at all. Also, we already know that the message handler hook is very inefficient as it has to look up the action based on its name (string). Why add this overhead to the RDMA operations if not needed? This kind of defeats the purpose. |
biddisco
force-pushed
the
action_customization
branch
3 times, most recently
from
a69bf54
to
90fe626
Compare
|
Well, improving the message handler hook might be something to look into then. I am just saying that this might be the perfect place to hook those RDMA operations in. It is about handling messages in a special way. The overhead of creating a parcel should be very minimal (just one allocation for the transfer action), despite the advantages you have when having a parcel: calculated message size, zero copy buffers and the target GID properly reference counted. |
biddisco
force-pushed
the
action_customization
branch
3 times, most recently
from
a9f9466
to
2ec2b85
Compare
Yes, this is on my list of things to do. Doing the hooking based on a type trait instead of the action name looks like the right thing to so. However, handling the RDMA actions further down the chain requires to create a parcel instance which is not necessary in the first place. We don't need any transfer actions either as we want to circumvent the need for any serialization. That means there is no need to calculate the message sizes (would be trivial for the very selected set of arguments we would support anyways), no need to have zero-copy buffers either. I admit, I don't completely understand the consequences wrt the target GID, though. |
biddisco
force-pushed
the
action_customization
branch
from
2ec2b85
to
29aceaa
Compare
|
@sithhell would you be ok now to go ahead and merge this? |
|
I use this customization point in my testing with RMA stuff, so I'd like to see it added to 1.2.0 |
|
As mentioned before, I have been using this customization point in my RDMA branch, so when that get's merged, I'll revisit the problem. For now, we can delete this PR and forget about it. |
…e.g. rdma)