-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Conversation
Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS. |
There was a problem hiding this 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.
system/lib/pthread/library_pthread.c
Outdated
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
Great, I'll write a test. |
This PR is now ready for review. I wrote the test and CI is passing. |
@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? |
The use case for this is probably covered by the new proxying API introduced in #15737 |
@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. |
Oh good, that's what I hoped! And sorry again for forgetting about this PR. |
@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! |
@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 |
FWIW if we added that functionality to the API, it would be implemented with basically the same |
@luigifcruz in your API, is it actually common/useful to have things like If so, the macros you have there don't look to unreasonable to me actually.. perhaps they could be called something like |
This pull-request adds the ability to make a synchronous dispatch to thread. This PR doesn't break the API. Address #13287.