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.

Sign up for GitHub

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

2027: Create Runnable inside Objgroup's invoke #2039

Merged
merged 7 commits into from
Feb 8, 2023
Merged

2027: Create Runnable inside Objgroup's invoke #2039

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
137536f
#2027: Tests: Add test to reproduce Non-copyable invoke call
JacobDomagala Dec 7, 2022
48c96ce
#2027: Objgroup: Add separate invoke functions for different return t…
JacobDomagala Dec 7, 2022
2fa7392
#2027: Objgroup: Use withObjGroup for runnables in ObjGroup
JacobDomagala Feb 2, 2023
caa92ea
#2027: ObjGroup: Use if constexpr to avoid multiple functions in ObjG…
JacobDomagala Feb 3, 2023
6043acd
#2027: Objgroup: Inline checks for invoke's return type
JacobDomagala Feb 7, 2023
703a972
#2027: invoke: refactor greatly for C++17
lifflander Feb 8, 2023
8820ec9
#2027: runnable: clever workaround to avoid branching on return type
lifflander Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/vt/objgroup/manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@

#include "vt/config.h"
#include "vt/runtime/component/component_pack.h"
#include "vt/utils/static_checks/function_ret_check.h"
#include "vt/objgroup/common.h"
#include "vt/objgroup/manager.fwd.h"
#include "vt/objgroup/proxy/proxy_objgroup.h"
Expand Down Expand Up @@ -234,7 +235,7 @@ struct ObjGroupManager : runtime::component::Component<ObjGroupManager> {
* \param[in] args function arguments
*/
template <typename ObjT, typename Type, Type f, typename... Args>
decltype(auto) invoke(ProxyElmType<ObjT> proxy, Args&&... args);
auto invoke(ProxyElmType<ObjT> proxy, Args&&... args);

/**
* \internal \brief Broadcast a message to all nodes in object group
Expand Down
37 changes: 30 additions & 7 deletions src/vt/objgroup/manager.impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,20 +211,43 @@ void ObjGroupManager::invoke(
}

template <typename ObjT, typename Type, Type f, typename... Args>
decltype(auto)
auto
ObjGroupManager::invoke(ProxyElmType<ObjT> proxy, Args&&... args) {
auto const dest_node = proxy.getNode();
auto const this_node = theContext()->getNode();

vtAssert(
dest_node == this_node,
fmt::format(
"Attempting to invoke handler on node:{} instead of node:{}!\n", this_node,
dest_node
)
);

return runnable::invoke<Type, f>(get(proxy), std::forward<Args>(args)...);
"Attempting to invoke handler on node:{} instead of node:{}!\n",
this_node, dest_node));

using Ret = typename util::FunctionWrapper<Type>::ReturnType;

if constexpr (not std::is_same_v<Ret, void>) {
Ret result;

runnable::makeRunnableVoid(false, uninitialized_handler, this_node)
.withObjGroup(get(proxy))
.runLambda([&] {
if constexpr (std::is_copy_constructible_v<Ret>) {
result =
runnable::invoke<Type, f>(get(proxy), std::forward<Args>(args)...);
} else {
auto&& ret =
runnable::invoke<Type, f>(get(proxy), std::forward<Args>(args)...);
result = std::move(ret);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

Does this all do the correct thing if f returns an l-value reference to a moveable but non-copyable type? Without looking at FunctionWrapper, if Ret preserves the reference, then Ret result just won't compile, I think.

My concern is that we may get back a reference to something that needs to be left alone, and then move from it.

Copy link
Member

Choose a reason for hiding this comment

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

More deeply, is there a reason that runLambda doesn't itself pass through the return value from the thing it runs, such that we could do

return runnable::makeRunnableVoid(...).runLambda([] { 
  return runnable::invoke<Type, f>(get(proxy), forward(args));
});

If it needs to manage making finish calls on the various context bits, those could be move to a scope guard structure that will call them in their destructors, rather than in straight-line code, so that we can return without declaring temporaries that would need to be conscientious about reference types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. Now that runLambda does it immediately we can push the return value through


return result;
} else {
runnable::makeRunnableVoid(false, uninitialized_handler, this_node)
.withObjGroup(get(proxy))
.runLambda([&] {
runnable::invoke<Type, f>(get(proxy), std::forward<Args>(args)...);
});
}
}


Expand Down
11 changes: 11 additions & 0 deletions tests/unit/objgroup/test_objgroup.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,17 @@ TEST_F(TestObjGroup, test_proxy_invoke) {

EXPECT_EQ(accumulate_result, 11);
EXPECT_EQ(proxy.get()->recv_, 2);

// Non-copyable
std::unique_ptr<int32_t> s{};
auto result = proxy[this_node]
.invoke<
decltype(&MyObjA::modifyNonCopyableStruct),
&MyObjA::modifyNonCopyableStruct>(std::move(s));

EXPECT_TRUE(result);
EXPECT_EQ(*result, 10);
EXPECT_EQ(proxy.get()->recv_, 3);
}

TEST_F(TestObjGroup, test_pending_send) {
Expand Down
7 changes: 7 additions & 0 deletions tests/unit/objgroup/test_objgroup_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,13 @@ struct MyObjA {
return std::accumulate(std::begin(vec), std::end(vec), 0);
}

std::unique_ptr<int32_t> modifyNonCopyableStruct(std::unique_ptr<int32_t> i) {
recv_++;
i = std::make_unique<int32_t>(10);

return i;
}

int id_ = -1;
int recv_ = 0;
static int next_id;
Expand Down