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

Add async-object #1339

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add async-object #1339

wants to merge 12 commits into from

Conversation

kirkshoop
Copy link
Contributor

@kirkshoop kirkshoop commented May 23, 2024

This adds async-object as proposed in p2849r0.

There are a few issues with this PR

The worst issue is that test_stop_object2.cpp takes 4m to compile. I had to make three cpp files with one test each to get that. When one cpp had all three test cases the compiler never completed. I did add -ftime-trace and loaded the 97mb json produced for test_stop_object2.cpp. It might be the std::visit/std::variant/std::tuple usage that is exploding compile times, but I do not have a solution yet.

The other issue is that the build has warnings:

In file included from /Users/kirkshoop/source/stdexec/test/exec/async_object/test_stop_object1.cpp:2:
In file included from /Users/kirkshoop/source/stdexec/include/exec/stop_object.hpp:28:
In file included from /Users/kirkshoop/source/stdexec/include/exec/async_using.hpp:25:
/Users/kirkshoop/source/stdexec/include/exec/__detail/__decl_receiver.hpp:44:15: warning: function 'exec::tag_invoke<stdexec::__get_env::get_env_t>' has internal linkage but is not defined [-Wundefined-internal]
  friend _Env tag_invoke(_Tag, const __t& __rcvr) noexcept;
              ^
/Users/kirkshoop/source/stdexec/include/stdexec/__detail/__tag_invoke.hpp:104:16: note: used here
        return tag_invoke(static_cast<_Tag&&>(__tag), static_cast<_Args&&>(__args)...);
               ^
393 warnings generated.

I am pretty sure that these are spurious since the __decl_receiver type is only used to test for nothrow connect and that is why it does not implement the functions.

Copy link

copy-pr-bot bot commented May 23, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@kirkshoop
Copy link
Contributor Author

Looks like Tim Song reported this as a spurious warning:

llvm/llvm-project#61566

@ericniebler
Copy link
Collaborator

/ok to test

@ericniebler
Copy link
Collaborator

/ok to test

Copy link
Collaborator

@ericniebler ericniebler left a comment

Choose a reason for hiding this comment

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

i haven't finished my review yet, but here are a few comments.

include/exec/__detail/__decl_receiver.hpp Outdated Show resolved Hide resolved
include/exec/async_object.hpp Outdated Show resolved Hide resolved
Comment on lines +86 to +110
auto async_destruct(storage& stg) noexcept {
auto make_destruct = [&] () noexcept -> exec::variant_sender<__destruction, __ref_storage> {
if (stg.__fyn_.has_value()) {
return stdexec::__apply(
[&](typename stdexec::__t<_FynId>&&... __fyn) noexcept {
return stdexec::__apply(
[&](typename stdexec::__t<_FynId>::storage&... __stgn) noexcept {
return stdexec::__apply(
[&](auto&&... __d) noexcept {
return stdexec::when_all(
stdexec::just(std::ref(stg)),
__d...);
}, exec::__tuple_reverse(std::make_tuple(exec::async_destruct(__fyn, __stgn)...)));
}, stg.__stgn_);
}, std::move(stg.__fyn_.value()));
} else {
return stdexec::just(std::ref(stg));
}
};
auto od = stdexec::then(make_destruct(), [](storage& stg) noexcept {
stg.o.reset();
stg.__fyn_.reset();
});
return od;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

hooo boy i can see why this takes a long time to compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that expression cannot be reduced using a function-ptr.

However, switching to member functions did make the compilation time much better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're using stdexec::__apply, which means you're using std::tuple. You might be able to improve compile times a little bit by using stdexec::__tup::__tuple.

include/exec/async_using.hpp Outdated Show resolved Hide resolved
@ericniebler
Copy link
Collaborator

/ok to test

@ericniebler
Copy link
Collaborator

/ok to test

@ericniebler
Copy link
Collaborator

/ok to test

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.

2 participants