521 shared msg lambda elim #550
Conversation
|
@pnstickne Random question, how do you manage to create PRs in "draft" mode. I can't find the way to do that! |
|
@lifflander Click the little "down arrow" in the right of the PR button - it's a context drop down. Not the most clear. |
src/vt/messaging/message/smart_ptr.h
Outdated
| std::is_trivially_destructible<T>(), | ||
| "Message shall not be downcast unless trivially destructible" | ||
| ); | ||
| // Establish type-erased deref if needed. |
There was a problem hiding this comment.
Could just 'always' pass/down use the deref proxy, especially since such are created with global lifetimes.
src/vt/messaging/message/smart_ptr.h
Outdated
| return MsgSharedPtr<U>{*this}; | ||
| return MsgSharedPtr<U>( | ||
| reinterpret_cast<U*>(ptr_), true, | ||
| deref_ ? deref_ : &statics::StaticMsgDerefs<T>); |
There was a problem hiding this comment.
Basically zero cost here to "create" the proxy.. and if counting CPU cycles, more branches might add up to more.
| @@ -49,7 +49,7 @@ namespace vt { namespace messaging { | |||
|
|
|||
| void PendingSend::sendMsg() { | |||
| if (send_action_ == nullptr) { | |||
| theMsg()->sendMsgSized(msg_.getShared(), msg_size_); | |||
| theMsg()->sendMsgSized(msg_, msg_size_); | |||
There was a problem hiding this comment.
Uh oh. "Feels" like there should be a to or the deref proxy should be universally consistent.
There was a problem hiding this comment.
Anyway, to will have the same effect as getShared for a non-trivially-destructible type.
|
@lifflander https://github.blog/2019-02-14-introducing-draft-pull-requests/ Also, ready to go in! |
I've no problem with such - although I've been scolded before. Amended the commit message for the typo (and improved the explanation in general). |
pnstickne
force-pushed
the
521-shared-msg-lambda-elim
branch
from
af56282 to
5e54643
Compare
|
Now the message-deref will always be done on the original type, regardless of if a type is trivially destructible or not (so there is no special case at all). Simplifies some checks (replaces one or more conditionals with a single virtual dispatch) and promotes generalized type-erasure support. Only results in more [fast] t-instantiations for message types that do not use |
|
@pnstickne Did you try running valgrind after these changes on some examples or tests to make sure that we are not leaking memory as a check? |
Nope. I probably should get in the habit :| |
- Folded VirtualMsgPtr into MsgSharedPtr making 'to' and 'toVirtual' synonyms. If the wrapped message type has a non-trivial destructor then a proxy 'deref' object is is used so that all derefs will apply to the original T-type (and thus invoke the delete-expression appropriately). This type-erasure ensures that 'messageDeref' is always invoked upon the correct T for expeted delete-expr behavior. - Remove unused code and unified methods - Ensure move contructor and added move-assignment operator (steal resources!!!!) - Remove messageDeref<Msg<T>> templates; if having a MsgSharedPtr, use it..
- Reduce new-delete (and associated complexity) through use of static template objects. - Use 'assert' for developer-code asserts (critical flaws, not logic usages).
- Copy, move, and such.. should all look the same, as easier to visualize by proximity. - Improve comments and such.
- Envelop deref returns ref value (post-deref). - Ordering, comments, type names, eg.
- Instead of 'sometimes using' an implementation, which involves several scattered conditional checks, the implementation is always used. Then "to" or "toVirtual" function as a virtue of propogating the object for the original Message type. Objects all exist with a static lifetime so there is effectively no additional cost (beyond the potential for a few more [cheap] template instantiations in cases where `to` or `toVirtual` where never invoked upon the original MsgT). - Renamed types and variables to reflect the larger scope; while messageDeref is the only current method, the implementation can be expanded (even more of a pimpl..) without name violation.
lifflander
force-pushed
the
521-shared-msg-lambda-elim
branch
from
aadb7e0 to
39ac08e
Compare
No description provided.