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

[Proxying] Add emscripten_proxy_callback_with_ctx #18810

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

tlively
Copy link
Member

@tlively tlively commented Feb 21, 2023

This new proxying function is like emscripten_proxy_callback, but supports
proxying of asynchronous work just like emscripten_proxy_sync_with_ctx. It
uses the existing em_proxying_ctx type and emscripten_proxy_finish function
to mark the work done, and that type and function are internally augmented to
handle both synchronous and callback-based proxying. emscripten_proxy_callback
is reimplemented to share most of its implementation with
emscripten_proxy_callback_with_ctx, but it does break the abstraction slightly
to avoid having to make an extra allocation.

@tlively tlively requested a review from sbc100 February 21, 2023 21:49
@tlively tlively force-pushed the proxying-cancel-callback branch from 848ee7d to 2213f1a Compare February 25, 2023 05:27
@tlively tlively force-pushed the proxying-callback-with-ctx branch from 1fe81f5 to 4baabe9 Compare February 25, 2023 05:27
@tlively tlively force-pushed the proxying-cancel-callback branch from 2213f1a to 8b7eadf Compare February 25, 2023 06:41
@tlively tlively force-pushed the proxying-callback-with-ctx branch from 4baabe9 to 6a5e4e7 Compare February 25, 2023 06:41
@tlively tlively force-pushed the proxying-cancel-callback branch from 8b7eadf to d67be45 Compare February 25, 2023 06:43
@tlively tlively force-pushed the proxying-callback-with-ctx branch from 6a5e4e7 to 5cb7113 Compare February 25, 2023 06:44
@tlively tlively force-pushed the proxying-cancel-callback branch from d67be45 to aaa19c3 Compare February 27, 2023 15:35
@tlively tlively force-pushed the proxying-callback-with-ctx branch from 5cb7113 to da1f95e Compare February 27, 2023 15:35
@tlively tlively force-pushed the proxying-callback-with-ctx branch from da1f95e to 289203e Compare February 28, 2023 15:39
@tlively tlively force-pushed the proxying-cancel-callback branch from a1353b4 to a5b591a Compare March 1, 2023 01:09
@tlively tlively force-pushed the proxying-callback-with-ctx branch from 289203e to 2ef3195 Compare March 1, 2023 01:10
@tlively tlively force-pushed the proxying-cancel-callback branch from a5b591a to 32309a3 Compare March 1, 2023 16:38
@tlively tlively force-pushed the proxying-callback-with-ctx branch from 2ef3195 to 4ccf2ee Compare March 1, 2023 16:38
@tlively tlively force-pushed the proxying-cancel-callback branch from 32309a3 to 26bcd74 Compare March 1, 2023 19:37
@tlively tlively force-pushed the proxying-callback-with-ctx branch from 4ccf2ee to 1f0ecb4 Compare March 1, 2023 19:37
@tlively tlively force-pushed the proxying-cancel-callback branch from 26bcd74 to 413728b Compare March 1, 2023 21:07
@tlively tlively force-pushed the proxying-callback-with-ctx branch from 1f0ecb4 to af490a7 Compare March 1, 2023 21:07
Base automatically changed from proxying-cancel-callback to main March 1, 2023 22:30
This new proxying function is like `emscripten_proxy_callback`, but supports
proxying of asynchronous work just like `emscripten_proxy_sync_with_ctx`. It
uses the existing `em_proxying_ctx` type and `emscripten_proxy_finish` function
to mark the work done, and that type and function are internally augmented to
handle both synchronous and callback-based proxying. `emscripten_proxy_callback`
is reimplemented to share most of its implementation with
`emscripten_proxy_callback_with_ctx`, but it does break the abstraction slightly
to avoid having to make an extra allocation.
@tlively tlively force-pushed the proxying-callback-with-ctx branch from af490a7 to 5415a72 Compare March 1, 2023 22:33
@tlively
Copy link
Member Author

tlively commented Mar 2, 2023

@sbc100 this is good to go besides review.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % naming .. which we can bikeshed later.

// `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.

@tlively tlively enabled auto-merge (squash) March 2, 2023 01:31
@tlively tlively merged commit db51a47 into main Mar 2, 2023
@tlively tlively deleted the proxying-callback-with-ctx branch March 2, 2023 01:36
impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023
…18810)

This new proxying function is like `emscripten_proxy_callback`, but supports
proxying of asynchronous work just like `emscripten_proxy_sync_with_ctx`. It
uses the existing `em_proxying_ctx` type and `emscripten_proxy_finish` function
to mark the work done, and that type and function are internally augmented to
handle both synchronous and callback-based proxying. `emscripten_proxy_callback`
is reimplemented to share most of its implementation with
`emscripten_proxy_callback_with_ctx`, but it does break the abstraction slightly
to avoid having to make an extra allocation.
impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023
…18810)

This new proxying function is like `emscripten_proxy_callback`, but supports
proxying of asynchronous work just like `emscripten_proxy_sync_with_ctx`. It
uses the existing `em_proxying_ctx` type and `emscripten_proxy_finish` function
to mark the work done, and that type and function are internally augmented to
handle both synchronous and callback-based proxying. `emscripten_proxy_callback`
is reimplemented to share most of its implementation with
`emscripten_proxy_callback_with_ctx`, but it does break the abstraction slightly
to avoid having to make an extra allocation.
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