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

[Proxying] Add emscripten_proxy_callback_with_ctx #18810

Merged
merged 1 commit into from
Mar 2, 2023
+505 −165
Merged

[Proxying] Add emscripten_proxy_callback_with_ctx #18810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
41 changes: 29 additions & 12 deletions site/source/docs/api_reference/proxying.h.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,9 @@ Functions

The same as ``emscripten_proxy_sync`` except that instead of waiting for the
proxied function to return, it waits for the proxied task to be explicitly
marked finished with ``emscripten_proxying_finish``. ``func`` need not call
``emscripten_proxying_finish`` itself; it could instead store the context
pointer and call ``emscripten_proxying_finish`` at an arbitrary later time.
marked finished with ``emscripten_proxy_finish``. ``func`` need not call
``emscripten_proxy_finish`` itself; it could instead store the context pointer
and call ``emscripten_proxy_finish`` at an arbitrary later time.

.. c:function:: int emscripten_proxy_callback(em_proxying_queue* q, pthread_t target_thread, void (*func)(void*), void (*callback)(void*), void (*cancel)(void*), void* arg)

Expand All @@ -103,6 +103,16 @@ Functions
receive the same argument, ``arg``. Returns 1 if ``func`` was successfully
enqueued and the target thread notified or 0 otherwise.

.. c:function:: int emscripten_proxy_callback_with_ctx(em_proxying_queue* q, pthread_t target_thread, void (*func)(em_proxying_ctx*, void*), void (*callback)(void*), void (*cancel)(void*), void* arg)

Enqueue ``func`` on the given queue and thread. Once (and if) it finishes the
task by calling ``emscripten_proxy_finish`` on the given ``em_proxying_ctx``,
it will asynchronously proxy ``callback`` back to the current thread on the
same queue, or if the target thread dies before the work can be completed,
``cancel`` will be proxied back instead. All three function will receive the
same argument, ``arg``. Returns 1 if ``func`` was successfully enqueued and
the target thread notified or 0 otherwise.

C++ API
-------

Expand Down Expand Up @@ -134,22 +144,29 @@ defined within namespace ``emscripten``.
Calls ``emscripten_proxy_async`` to execute ``func``, returning ``true`` if the
function was successfully enqueued and ``false`` otherwise.

.. cpp:member:: bool proxyCallback(pthread_t target, std::function<void()>&& func, std::function<void()>&& callback, std::function<void()>&& cancel)

Calls ``emscripten_proxy_callback`` to execute ``func`` and schedule either
``callback`` or ``cancel``, returning ``true`` if the function was
successfully enqueued and ``false`` otherwise.

.. cpp:member:: bool proxySync(const pthread_t target, const std::function<void()>& func)

Calls ``emscripten_proxy_sync`` to execute ``func``, returning ``true`` if the
function was successfully completed or ``false`` otherwise.

.. cpp:member:: bool proxySyncWithCtx(const pthread_t target, const std::function<void(ProxyingCtx)>& func)

Calls ``emscripten_proxy_sync_with_ctx`` to execute ``func``, returning ``true``
if the function was successfully marked done with
``emscripten_proxying_finish`` or ``ProxyingCtx::finish`` and ``false`` otherwise.
Calls ``emscripten_proxy_sync_with_ctx`` to execute ``func``, returning
``true`` if the function was successfully marked done with
``emscripten_proxy_finish`` or ``ProxyingCtx::finish`` and ``false``
otherwise.

.. cpp:member:: bool proxyCallback(pthread_t target, std::function<void()>&& func, std::function<void()>&& callback, std::function<void()>&& cancel)

Calls ``emscripten_proxy_callback`` to execute ``func`` and schedule either
``callback`` or ``cancel``, returning ``true`` if the function was
successfully enqueued and ``false`` otherwise.

.. cpp:member:: bool proxyCallbackWithCtx(pthread_t target, std::function<void(ProxyingCtx)>&& func, std::function<void()>&& callback, std::function<void()>&& cancel)

Calls ``emscripten_proxy_callback_with_ctx`` to execute ``func`` and
schedule either ``callback`` or ``cancel``, returning ``true`` if the
function was successfully enqueued and ``false`` otherwise.

.. _proxying.h: https://github.com/emscripten-core/emscripten/blob/main/system/include/emscripten/proxying.h
.. _test_pthread_proxying.c: https://github.com/emscripten-core/emscripten/blob/main/test/pthread/test_pthread_proxying.c
Expand Down
97 changes: 81 additions & 16 deletions system/include/emscripten/proxying.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,12 @@ int emscripten_proxy_sync(em_proxying_queue* q,
void* arg);

// Enqueue `func` on the given queue and thread and wait for it to be executed
// and for the task to be marked finished with `emscripten_proxying_finish`
// before returning. `func` need not call `emscripten_proxying_finish` itself;
// it could instead store the context pointer and call
// `emscripten_proxying_finish` at an arbitrary later time. Returns 1 if the
// task was successfully completed and 0 otherwise, including if the target
// thread is canceled or exits before the work is completed.
// and for the task to be marked finished with `emscripten_proxy_finish` before
// returning. `func` need not call `emscripten_proxy_finish` itself; it could
// instead store the context pointer and call `emscripten_proxy_finish` at an
// arbitrary later time. Returns 1 if the task was successfully completed and 0
// otherwise, including if the target thread is canceled or exits before the
// work is completed.
int emscripten_proxy_sync_with_ctx(em_proxying_queue* q,
pthread_t target_thread,
void (*func)(em_proxying_ctx*, void*),
Expand All @@ -88,6 +88,20 @@ int emscripten_proxy_callback(em_proxying_queue* q,
void (*cancel)(void*),
void* arg);

// Enqueue `func` on the given queue and thread. Once (and if) it finishes the
// task by calling `emscripten_proxy_finish` on the given `em_proxying_ctx`, it
// will asynchronously proxy `callback` back to the current thread on the same
// queue, or if the target thread dies before the work can be completed,
// `cancel` will be proxied back instead. All three function will receive the
// same argument, `arg`. Returns 1 if `func` was successfully enqueued and the
// target thread notified or 0 otherwise.
int emscripten_proxy_callback_with_ctx(em_proxying_queue* q,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just called this emscripten_proxy_async_with_ctx?

I think maybe the _with_callback distinction is important, and also obvious the from the args. Ideally I think we could remove the _with_callback variants that just have the normal versions take optional callbacks in all cases.

Another reason is that all the current function start with either emscripten_proxy_sync or emscripten_proxy_async

Copy link
Member Author

Choose a reason for hiding this comment

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

After this full sequence of PRs, we have :

emscripten_proxy_async
emscripten_proxy_sync{_with_ctx}
emscripten_proxy_callback{_with_ctx}
emscripten_proxy_promise{_with_ctx}

If we really wanted to minimize the API surface, my preference would be to remove emscripten_proxy_async and emscripten_proxy_callback*, leaving only sync and promise proxying, with and without ctxs. The reason I haven't tried to do that minimization yet is that it seems low cost to expose the extra functions if we're going to have nearly identical functions under the hood either way. Users get the most options at negligible maintenance cost to us.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds reasonable.

As a general rule, I would rather keep the low level C API as small as possible. For this kind of low level API (which I hope most users will never need to use), I think its fine if we don't include helpers/wrappers/shortcuts.

If we want to provide them in C++ API, or in a higher level set of C APIS, that would be fine, but at least for me it helper conceptually to keep this surface small... but maybe thats just me.

None of this has to block this change.. I don't think refactoring this layer later on is very risky.

pthread_t target_thread,
void (*func)(em_proxying_ctx*, void*),
void (*callback)(void*),
void (*cancel)(void*),
void* arg);

#ifdef __cplusplus
} // extern "C"

Expand All @@ -103,6 +117,18 @@ namespace emscripten {

// A thin C++ wrapper around the underlying C API.
class ProxyingQueue {
public:
// Simple wrapper around `em_proxying_ctx*` providing a `finish` method as an
// alternative to `emscripten_proxy_finish`.
struct ProxyingCtx {
em_proxying_ctx* ctx;

ProxyingCtx() = default;
ProxyingCtx(em_proxying_ctx* ctx) : ctx(ctx) {}
void finish() { emscripten_proxy_finish(ctx); }
};

private:
static void runAndFree(void* arg) {
auto* f = (std::function<void()>*)arg;
(*f)();
Expand Down Expand Up @@ -150,6 +176,37 @@ class ProxyingQueue {
delete info;
}

struct CallbackWithCtxFuncs {
std::function<void(ProxyingCtx)> func;
std::function<void()> callback;
std::function<void()> cancel;

CallbackWithCtxFuncs(std::function<void(ProxyingCtx)>&& func,
std::function<void()>&& callback,
std::function<void()>&& cancel)
: func(std::move(func)), callback(std::move(callback)),
cancel(std::move(cancel)) {}
};

static void runFuncWithCtx(em_proxying_ctx* ctx, void* arg) {
auto* info = (CallbackWithCtxFuncs*)arg;
info->func(ProxyingCtx{ctx});
}

static void runCallbackWithCtx(void* arg) {
auto* info = (CallbackWithCtxFuncs*)arg;
info->callback();
delete info;
}

static void runCancelWithCtx(void* arg) {
auto* info = (CallbackWithCtxFuncs*)arg;
if (info->cancel) {
info->cancel();
}
delete info;
}

public:
em_proxying_queue* queue = em_proxying_queue_create();

Expand Down Expand Up @@ -179,16 +236,6 @@ class ProxyingQueue {
}
}

// Simple wrapper around `em_proxying_ctx*` providing a `finish` method as an
// alternative to `emscripten_proxy_finish`.
struct ProxyingCtx {
em_proxying_ctx* ctx;

ProxyingCtx() = default;
ProxyingCtx(em_proxying_ctx* ctx) : ctx(ctx) {}
void finish() { emscripten_proxy_finish(ctx); }
};

void execute() { emscripten_proxy_execute_queue(queue); }

// Return true if the work was successfully enqueued and false otherwise.
Expand Down Expand Up @@ -225,6 +272,24 @@ class ProxyingQueue {
}
return true;
}

bool proxyCallbackWithCtx(pthread_t target,
std::function<void(ProxyingCtx)>&& func,
std::function<void()>&& callback,
std::function<void()>&& cancel) {
CallbackWithCtxFuncs* info = new CallbackWithCtxFuncs(
std::move(func), std::move(callback), std::move(cancel));
if (!emscripten_proxy_callback_with_ctx(queue,
target,
runFuncWithCtx,
runCallbackWithCtx,
runCancelWithCtx,
info)) {
delete info;
return false;
}
return true;
}
};

} // namespace emscripten
Expand Down
Loading