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

Jump to bottom

Move coCapture() to libkj #82

Merged
merged 2 commits into from Oct 7, 2022
Merged

Move coCapture() to libkj #82

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c3a89e
Prepare to move coCapture() to libkj
harrishancock Oct 7, 2022
9f0666b
Update Cap'n Proto to use kj::coCapture()
harrishancock Oct 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/workerd/util/co-capture-test.c++
View file
Open in desktop
Expand Up @@ -7,15 +7,9 @@
#include <kj/async-io.h>
#include <kj/test.h>

#include <algorithm>
#include <random>

namespace workerd {
namespace {

std::random_device randomDevice;
std::mt19937 randomIterator(randomDevice());

KJ_TEST("Verify coCapture() functors can only be run once") {
auto io = kj::setupAsyncIo();

Expand Down
24 changes: 12 additions & 12 deletions src/workerd/util/co-capture.h
View file
Open in desktop
Expand Up @@ -4,48 +4,48 @@

#pragma once

#include <kj/debug.h>
#include <type_traits>
#include <kj/common.h>

namespace workerd {

namespace _ {

template<typename Functor>
struct CaptureForCoroutine {
kj::Maybe<Functor> maybeFunctor;

explicit CaptureForCoroutine(Functor&& f) : maybeFunctor(kj::mv(f)) {}

template<typename ...Args>
using ReturnType = std::invoke_result_t<Functor, Args...>;

template<typename ...Args>
static auto coInvoke(Functor functor, Args&&... args) -> ReturnType<Args&&...> {
static auto coInvoke(Functor functor, Args&&... args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the name functor here confuses me a lot.

As far as I remember the theory, in expression map(list, f) - map is a functor. f is a function.
Or more exactly: map(*, f) is a list->list functor. (https://en.wikipedia.org/wiki/Functor_(functional_programming)#:~:text=In%20functional%20programming%2C%20a%20functor,in%20Haskell%20using%20type%20class)

So in this context CaptureForCoroutine is a functor. And it operates on functions.

Wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with you, but the misnomer is already quite prevalent in this codebase. I refrained from changing the names (I prefer "function object", or maybe nowadays "invokable", myself) to avoid opening the can of worms. :)

Copy link
Member

Choose a reason for hiding this comment

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

It is a misnomer, but it's a broader C++ community misnomer, not one unique to this code. That wiki page itself hints at why it ended up getting used here:

In C++, the name functor is commonly used to refer to a function object, even though the ISO/IEC 14882 standard specification itself exclusively uses the latter term.

I don't think anyone would be too upset if you just renamed this to fn though.

-> decltype(functor(kj::fwd<Args>(args)...)) {
// Since the functor is now in the local scope and no longer a member variable, it will be
// persisted in the coroutine state.

// Note that `co_await localFunctor(...)` can still return `void`. It just happens that
// Note that `co_await functor(...)` can still return `void`. It just happens that
// `co_return voidReturn();` is explicitly allowed.
co_return co_await functor(kj::fwd<Args>(args)...);
}

template<typename ...Args>
auto operator()(Args&&... args) {
auto localFunctor = kj::mv(KJ_REQUIRE_NONNULL(
maybeFunctor, "Attempted to invoke CaptureForCoroutine functor multiple times"));
KJ_IREQUIRE(maybeFunctor != nullptr,
"Attempted to invoke CaptureForCoroutine functor multiple times");
auto localFunctor = kj::mv(*kj::_::readMaybe(maybeFunctor));
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not KJ_IF_MAYBE? Can't you write the same code using it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just a microoptimization that the compiler could also probably figure out. If I used a KJ_IF_MAYBE, I would be adding a redundant nullness check.

maybeFunctor = nullptr;
return coInvoke(kj::mv(localFunctor), kj::fwd<Args>(args)...);
}
};

} // namespace _

template <typename Functor>
auto coCapture(Functor&& f) {
// Wrap a functor with a lambda that stores it on the heap, invokes it, and then attaches the
// functor to the result promise.
// Assuming `f()` returns a Promise<T> `p`, wrap `f` in such a way that it will outlive its
// returned Promise. Note that the returned object may only be invoked once.
//
// This function is meant to help address this pain point with functors that return a coroutine:
// https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#cp51-do-not-use-capturing-lambdas-that-are-coroutines
// https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rcoro-capture
//
// The two most common patterns where this may be useful look like so:
// ```
Expand Down