[Proxying] Update proxying with callback to support cancellation by tlively · Pull Request #18776 · emscripten-core/emscripten

tlively · 2023-02-16T21:34:44Z

Completely overhaul the interface for proxying with callbacks:

Rename emscripten_proxy_async_with_callback to the simpler emscripten_proxy_callback.
Add an optional cancel callback argument that will be scheduled on the proxying thread if the worker thread dies before the work is started. A future PR will change this so that the cancel callback will be called if a worker thread dies before the work is finished to be more consistent with the proxy_sync APIs.
Remove the ability to receive the result of the proxied function as an argument to the callback function since the extra expressiveness was not general enough to warrant the added complexity.
Use only a single context argument rather than a separate context for the proxied function and the callback. This is just as expressive, but it is simpler avoids the question of what context object should be passed to the cancellation callback.

Also update the only user of the callback API, dynlink.c, to use the new API. A future PR will implement a promise-based proxying API and update dynlink.c to use that instead, so this PR just tries to make the minimal necessary changes to dynlink.c.

sbc100 · 2023-02-16T21:47:29Z

+  struct promise_result* info = malloc(sizeof(struct promise_result));
+  if (!info) {
+    return false;
+  }


Seems a little unfortunate to have to add an extra allocation here... but I guess it keeps the interface simpler?

sbc100 · 2023-02-16T21:48:48Z

+  if (!info) {
+    return false;
+  }
+  *info = (struct promise_result){.promise = promise, .result = false};


I think find it little more readable to just do:

info->promise = promise; info->result = false;

Is the cast here actually needed? I guess so?

Yeah, it's not actually a cast, but rather part of the compound literal syntax.

In general I prefer this syntax, but I agree that it's nice to give each field its own line. Maybe I'll try to tweak the clang format settings to make this nicer.

It turns out that there's no clang-format setting for this, but putting a comma after the last item achieves the same thing.

It just seems like more, and less obvious, syntax than the simple pair of assignments.. but maybe I'm just not familiar with reading code with this syntax.

tlively · 2023-02-16T22:04:08Z

This depends on #18741, by the way.

tlively · 2023-02-16T22:23:32Z

Current dependencies on/for this PR:

main
- PR [Proxying][NFC] Move deferred cleanup logic to em_task_queue #18851
  - PR [Proxying] Send messages via in-memory mailbox queues #18852
    - PR [Proxying] Cancel proxied work when the worker thread dies #18741
      - PR [Proxying] Update proxying with callback to support cancellation #18776 👈
        
        PR [Proxying] Add emscripten_proxy_callback_with_ctx #18810
        
        PR [Proxying] Add proxying functions that return promises #18825
        
        PR [Proxying] Use Atomics.waitAsync to receive proxying notifications #18861
        
        PR Remove unnecessary EXIT_RUNTIME setting from proxying tests #18873

This comment was auto-generated by Graphite.

Completely overhaul the interface for proxying with callbacks: - Rename `emscripten_proxy_async_with_callback` to the simpler `emscripten_proxy_callback`. - Add an optional `cancel` callback argument that will be scheduled on the proxying thread if the worker thread dies before the work is started. A future PR will change this so that the `cancel` callback will be called if a worker thread dies before the work is finished to be more consistent with the `proxy_sync` APIs. - Remove the ability to receive the result of the proxied function as an argument to the callback function since the extra expressiveness was not general enough to warrant the added complexity. - Use only a single context argument rather than a separate context for the proxied function and the callback. This is just as expressive, but it is simpler avoids the question of what context object should be passed to the cancellation callback. Also update the only user of the callback API, dynlink.c, to use the new API. A future PR will implement a promise-based proxying API and update dynlink.c to use that instead, so this PR just tries to make the minimal necessary changes to dynlink.c.

…cripten-core#18776) Completely overhaul the interface for proxying with callbacks: - Rename `emscripten_proxy_async_with_callback` to the simpler `emscripten_proxy_callback`. - Add an optional `cancel` callback argument that will be scheduled on the proxying thread if the worker thread dies before the work is started. A future PR will change this so that the `cancel` callback will be called if a worker thread dies before the work is finished to be more consistent with the `proxy_sync` APIs. - Remove the ability to receive the result of the proxied function as an argument to the callback function since the extra expressiveness was not general enough to warrant the added complexity. - Use only a single context argument rather than a separate context for the proxied function and the callback. This is just as expressive, but it is simpler avoids the question of what context object should be passed to the cancellation callback. Also update the only user of the callback API, dynlink.c, to use the new API. A future PR will implement a promise-based proxying API and update dynlink.c to use that instead, so this PR just tries to make the minimal necessary changes to dynlink.c.

DreamOfIce · 2023-04-04T16:42:27Z

@tlively @sbc100 I do need a callback to get the result returned by the function, is there any way to do this?

DreamOfIce · 2023-04-04T16:44:55Z

Also, changelog of this seems to be written to 3.1.32 instead of correct 3.1.33

tlively · 2023-04-04T17:01:02Z

@DreamOfIce, if you reserve space for the result in the memory block you pass as the arg parameter, then you can store the result there in func and retrieve it in callback, since the same arg parameter will be passed to both of them.

tlively · 2023-04-04T17:02:47Z

Also, changelog of this seems to be written to 3.1.32 instead of correct 3.1.33

Thanks for catching that. I'll fix it.

PR #18776 updated the changelog for 3.1.32, but it should have updated the changelog for 3.1.33, which was in development at the time.

tlively · 2023-04-04T17:05:24Z

#19132

PR #18776 updated the changelog for 3.1.32, but it should have updated the changelog for 3.1.33, which was in development at the time.

DreamOfIce · 2023-04-05T09:09:39Z

Also, any progress on the promise based proxy API yet? Is the promise here in js or cpp?

tlively · 2023-04-05T18:38:38Z

@DreamOfIce, yes, you can see it here: https://github.com/emscripten-core/emscripten/blob/main/system/include/emscripten/proxying.h#L106-L116.

Note that there is no documentation yet, because it relies on the promise API in promise.h, which isn't finished yet. There's also no C++ wrapper for emscripten_proxy_promise for the same reason.

DreamOfIce · 2023-04-06T03:12:23Z

In that case, I'll go ahead and use the version I implemented before

tlively requested a review from sbc100 February 16, 2023 21:34

sbc100 approved these changes Feb 16, 2023

View reviewed changes

tlively mentioned this pull request Feb 16, 2023

[Proxying] Cancel proxied work when the worker thread dies #18741

Merged

tlively force-pushed the proxying-shutdown branch from 741aafa to b9fb8c0 Compare February 16, 2023 22:26

tlively force-pushed the proxying-cancel-callback branch 2 times, most recently from f04027b to 60fd680 Compare February 17, 2023 05:47

tlively force-pushed the proxying-shutdown branch from 54d2274 to d13a814 Compare February 17, 2023 06:02

tlively force-pushed the proxying-cancel-callback branch from 60fd680 to 2d74803 Compare February 17, 2023 06:02

This was referenced Feb 21, 2023

[Proxying] Add emscripten_proxy_callback_with_ctx #18810

Merged

[Proxying] Add proxying functions that return promises #18825

Merged

tlively force-pushed the proxying-shutdown branch from d13a814 to 1301f0e Compare February 25, 2023 05:27

tlively force-pushed the proxying-cancel-callback branch from 848ee7d to 2213f1a Compare February 25, 2023 05:27

This was referenced Feb 25, 2023

[Proxying] Send messages via in-memory mailbox queues #18852

Merged

[Proxying][NFC] Move deferred cleanup logic to em_task_queue #18851

Merged

tlively force-pushed the proxying-shutdown branch from 1301f0e to c7918e5 Compare February 25, 2023 06:41

tlively force-pushed the proxying-cancel-callback branch from 2213f1a to 8b7eadf Compare February 25, 2023 06:41

tlively force-pushed the proxying-shutdown branch from c7918e5 to 5223090 Compare February 25, 2023 06:43

tlively force-pushed the proxying-cancel-callback branch from 8b7eadf to d67be45 Compare February 25, 2023 06:43

tlively force-pushed the proxying-shutdown branch from 5223090 to e0abb19 Compare February 27, 2023 15:35

tlively force-pushed the proxying-cancel-callback branch from d67be45 to aaa19c3 Compare February 27, 2023 15:35

This was referenced Feb 27, 2023

[Proxying] Use Atomics.waitAsync to receive proxying notifications #18861

Merged

Remove unnecessary EXIT_RUNTIME setting from proxying tests #18873

Merged

tlively force-pushed the proxying-shutdown branch from e0abb19 to 09a3df8 Compare March 1, 2023 01:09

tlively force-pushed the proxying-cancel-callback branch from a1353b4 to a5b591a Compare March 1, 2023 01:09

tlively force-pushed the proxying-shutdown branch from 09a3df8 to 8b4eb90 Compare March 1, 2023 16:38

tlively force-pushed the proxying-cancel-callback branch from a5b591a to 32309a3 Compare March 1, 2023 16:38

tlively force-pushed the proxying-shutdown branch from 8b4eb90 to ea71ab3 Compare March 1, 2023 19:37

tlively force-pushed the proxying-cancel-callback branch from 32309a3 to 26bcd74 Compare March 1, 2023 19:37

Base automatically changed from proxying-shutdown to main March 1, 2023 21:04

tlively added 3 commits March 1, 2023 13:06

fix minimal runtime build

c37b39e

re-enable dlopen asan test

413728b

tlively force-pushed the proxying-cancel-callback branch from 26bcd74 to 413728b Compare March 1, 2023 21:07

tlively merged commit 4589faa into main Mar 1, 2023

tlively deleted the proxying-cancel-callback branch March 1, 2023 22:30

tlively added a commit that referenced this pull request Apr 4, 2023

Move proxying change to correct location in ChangeLog.md

812214e

PR #18776 updated the changelog for 3.1.32, but it should have updated the changelog for 3.1.33, which was in development at the time.

tlively mentioned this pull request Apr 4, 2023

Move proxying change to correct location in ChangeLog.md #19132

Merged

tlively added a commit that referenced this pull request Apr 4, 2023

Move proxying change to correct location in ChangeLog.md (#19132)

b2f3b50

PR #18776 updated the changelog for 3.1.32, but it should have updated the changelog for 3.1.33, which was in development at the time.

Uh oh!

Conversation

tlively commented Feb 16, 2023

Uh oh!

sbc100 Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

tlively Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

tlively Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

tlively commented Feb 16, 2023

Uh oh!

tlively commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DreamOfIce commented Apr 4, 2023

Uh oh!

DreamOfIce commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively commented Apr 4, 2023

Uh oh!

tlively commented Apr 4, 2023

Uh oh!

tlively commented Apr 4, 2023

Uh oh!

DreamOfIce commented Apr 5, 2023

Uh oh!

tlively commented Apr 5, 2023

Uh oh!

DreamOfIce commented Apr 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tlively commented Feb 16, 2023 •

edited

Loading

DreamOfIce commented Apr 4, 2023 •

edited

Loading