Skip to content

Add synchronous dispatch to thread. #13315

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

Closed
wants to merge 11 commits into from

Conversation

luigifcruz
Copy link

@luigifcruz luigifcruz commented Jan 25, 2021

This pull-request adds the ability to make a synchronous dispatch to thread. This PR doesn't break the API. Address #13287.

@welcome
Copy link

welcome bot commented Jan 25, 2021

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Idea sounds reasonable, thanks for the PR!

Will need a test eventually.

int _emscripten_do_dispatch_to_thread(
pthread_t target_thread, em_queued_call* call) {
int _emscripten_do_dispatch_to_thread_global(
pthread_t target_thread, em_queued_call* call, int drop) {
Copy link
Member

Choose a reason for hiding this comment

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

drop isn't clear enough to me. what is it meant to do?

Copy link
Author

Choose a reason for hiding this comment

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

If 0 this will overwrite the default behaviour of this method to automatically drop calls if the target_thread isn't the main thread. In this case, dropping a call means free the em_queued_call object and don't wait the asynchronous call to return. I did it this way to make as few changes as possible to proven code.

Copy link
Member

Choose a reason for hiding this comment

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

The term "drop" still seems odd to me. It does do the call, correct, it just fires it off and forgets, basically?

Copy link
Author

@luigifcruz luigifcruz Feb 11, 2021

Choose a reason for hiding this comment

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

Yes, this is the default behavior if the target thread isn't the main thread. We can change the name to forceSync.

@luigifcruz
Copy link
Author

Great, I'll write a test.

Base automatically changed from master to main March 8, 2021 23:49
@luigifcruz luigifcruz requested a review from kripken March 14, 2021 20:26
@luigifcruz
Copy link
Author

This PR is now ready for review. I wrote the test and CI is passing.

@kripken
Copy link
Member

kripken commented Feb 4, 2022

@luigifcruz sorry, I seem to have missed this review request...

I think this might overlap with work @tlively has been doing on the thread dispatch APIs. Is it still needed?

@tlively
Copy link
Member

tlively commented Feb 4, 2022

The use case for this is probably covered by the new proxying API introduced in #15737

@luigifcruz
Copy link
Author

@kripken Looks like the new proxy API solves the problem that this PR is trying to resolve. I will rewrite my WebUSB to libusb translation layer using it. Thanks for the heads-up.

@luigifcruz luigifcruz closed this Feb 4, 2022
@kripken
Copy link
Member

kripken commented Feb 4, 2022

Oh good, that's what I hoped!

And sorry again for forgetting about this PR.

@tlively
Copy link
Member

tlively commented Feb 5, 2022

@luigifcruz you'll be the first user of that new API, so please let me know if you run into any issues or if you find anything that could be improved!

@luigifcruz
Copy link
Author

@tlively After a long hiatus, I finally had time to port my project to the new API. In short, it has a very intuitive interface and it worked pretty well! The only improvement I can think of is to issue a warning and call the proxied function in the current thread instead of a hard error. Or hide this functionality behind a flag. I had to implement an ugly if ... else to handle this circumstance.

@tlively
Copy link
Member

tlively commented Jul 10, 2023

FWIW if we added that functionality to the API, it would be implemented with basically the same if ... else and exposed as a separate method because it's important that users explicitly opt-in to having the proxied function called right away. For example, cases where the proxied function expects to be run from the event loop without anything on the stack or where it expects to be called only from a particular part of the application that calls emscripten_proxy_execute_queue would be broken if we automatically called the function immediately without opt-in. Maybe it's worth adding a emscripten_proxy_sync_or_call, though, since opting in would probably be common.

@sbc100
Copy link
Collaborator

sbc100 commented Jul 10, 2023

@luigifcruz in your API, is it actually common/useful to have things like em_proxy_get_device_list actually not proxy at all and just run in the local thread?

If so, the macros you have there don't look to unreasonable to me actually.. perhaps they could be called something like EM_MAYBE_PROXY to re-enforce that they don't always do it?

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.

4 participants