Skip to content

Refactor the threading.h C proxying API #15681

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 3 commits into from

Conversation

tlively
Copy link
Member

@tlively tlively commented Dec 1, 2021

Refactor the C proxying API to separate the proxying logic from the logic for
creation and use of em_queued_call objects. Also provide a richer and more
orthogonal set of emscripten_dispatch_to_thread_* variants:

  • emscripten_dispatch_to_thread - send a task to a thread for asynchronous
    execution.

  • emscripten_dispatch_to_thread_async - same, but schedule for asynchronous
    execution even if already on the target thread.

  • emscripten_dispatch_to_thread_sync - wait for the dispatched function to
    finish before returning.

  • emscripten_dispatch_to_thread_async_as_sync - wait for the dispatched task to
    be marked finished via an explicit call to emscripten_async_as_sync_finish.

Each of these functions comes in a lower-level *_ptr variant that accepts
function pointers and void* arguments as well as higher-level variants that
construct and dispatch em_queued_call objects.

Each variant builds incrementally on some other variant, all bottoming out at
emscripten_dispatch_to_thread_ptr, so it should be easy to incrementally
understand how each layer works. The other miscellaneous proxying functions
exposed in threading.h are now implemented in terms of the most appropriate
emscripten_dispatch_to_thread* variant as well.

This is the first step toward decoupling the logic for proxying work to threads
from the logic for creation and dispatch of `em_queued_call` objects, which
couple a dynamically typed function pointer with its arguments. Decoupling these
pieces will allow for more general proxying that is not required to use the
`em_queued_call` mechanism and will simplify the code.
Refactor the C proxying API to separate the proxying logic from the logic for
creation and use of em_queued_call objects. Also provide a richer and more
orthogonal set of `emscripten_dispatch_to_thread_*` variants:

 - emscripten_dispatch_to_thread - send a task to a thread for asynchronous
   execution.

 - emscripten_dispatch_to_thread_async - same, but schedule for asynchronous
   execution even if already on the target thread.

 - emscripten_dispatch_to_thread_sync - wait for the dispatched function to
   finish before returning.

 - emscripten_dispatch_to_thread_async_as_sync - wait for the dispatched task to
   be marked finished via an explicit call to `emscripten_async_as_sync_finish`.

Each of these functions comes in a lower-level `*_ptr` variant that accepts
function pointers and void* arguments as well as higher-level variants that
construct and dispatch `em_queued_call` objects.

Each variant builds incrementally on some other variant, all bottoming out at
`emscripten_dispatch_to_thread_ptr`, so it should be easy to incrementally
understand how each layer works. The other miscellaneous proxying functions
exposed in threading.h are now implemented in terms of the most appropriate
`emscripten_dispatch_to_thread*` variant as well.
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.

threading.h changes look nice!

Before I start to read the code, the description mentions both a refactoring and the addition of new things. Could this be split into a NFC refactoring in an initial PR?

@tlively
Copy link
Member Author

tlively commented Dec 1, 2021

I can split out a few things, yes. Others would be more difficult to split out because they are now implemented in terms of the new functions. I just reopened #15648 as a first step. I'll make another preliminary PR or two.

 - Rebaseline the failing metadce test
 - Change the expected return value of `emscripten_dispatch_to_thread`. It now
   returns 1 if the call was successfully scheduled, not just if it was
   synchronously executed.
@tlively
Copy link
Member Author

tlively commented Dec 1, 2021

Just added a commit to fix some tests. First, the code size increased and the exports changed in the metadce test. Second, emscripten_dispatch_to_thread now returns 1 if the call was successfully scheduled while previously it only returned 1 if the call was synchronously executed so a test expectation had to be updated.

I'm still investigating the posixtest.test_pthread_setcanceltype_1_1 failure.

@tlively
Copy link
Member Author

tlively commented Dec 1, 2021

Another preliminary NFC change: #15686

@tlively
Copy link
Member Author

tlively commented Dec 1, 2021

Another smaller change, this one not NFC: #15690

@tlively
Copy link
Member Author

tlively commented Dec 1, 2021

Another NFC change: #15691.

ethanalee added a commit that referenced this pull request Dec 3, 2021
Relevant Issue: #15041

Introduce new proxied backend structure for the new file system.
Proxying will be refactored soon using on this PR: #15681
@tlively
Copy link
Member Author

tlively commented Dec 9, 2021

Closing this in favor of the plan in #15705 (comment).

@tlively tlively closed this Dec 9, 2021
mmarczell-graphisoft pushed a commit to GRAPHISOFT/emscripten that referenced this pull request Jan 5, 2022
Relevant Issue: emscripten-core#15041

Introduce new proxied backend structure for the new file system.
Proxying will be refactored soon using on this PR: emscripten-core#15681
@tlively tlively deleted the proxying-cleanup branch February 5, 2024 18:04
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