Skip to content

Remove some of the complexity from __timedwait.c. NFC#26511

Open
sbc100 wants to merge 1 commit intoemscripten-core:mainfrom
sbc100:__timed_wait
Open

Remove some of the complexity from __timedwait.c. NFC#26511
sbc100 wants to merge 1 commit intoemscripten-core:mainfrom
sbc100:__timed_wait

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Mar 20, 2026

This should have been part of #26471.

The breaking up of the wait time for 2 of the 3 cases that are handled here is now handled one layer down in emscripten_futex_wait:

  1. Breaking up the wait because we are the main runtime thread.
  2. Breaking up the wait because we are async cancelable.

The third cases here (breaking up the wait because we are cancelable in the non-async sense) still needs to be handled here at the higher level.

@sbc100 sbc100 requested review from dschuff and kripken March 20, 2026 23:19
@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 20, 2026

What gemini had to say about this change:

✦ I have reviewed your proposed changes at HEAD. The changes in system/lib/libc/musl/src/thread/__timedwait.c appear to be a sound cleanup and
optimization.

Key Observations:

  1. Correctness Fix: The original code had a subtle bug where it could return ECANCELED on the main thread even if cancellation was disabled
    (because it entered the sliced-sleep loop due to is_runtime_thread being true, and then checked pthread_self()->cancel without verifying
    canceldisable again after __pthread_testcancel() returned). Your change avoids entering this loop when canceldisable ==
    PTHREAD_CANCEL_DISABLE, which is more correct.
  2. Responsiveness Preserved: Although you removed the explicit is_runtime_thread check and the 1ms slicing from __timedwait_cp, the main
    thread's responsiveness is still maintained. This is because emscripten_futex_wait (which __timedwait_cp calls) contains its own logic to
    slice waits into 1ms intervals and call _emscripten_yield when running on the main runtime thread.
  3. Code Size Reduction: The change successfully reduces the WASM binary size by approximately 32 bytes in the minimal pthreads test cases,
    as reflected in the updated codesize JSON files.
  4. Deferred Cancellation: For threads with deferred cancellation enabled, the response time to a cancellation request on the main thread
    will increase from 1ms to 100ms (the value of max_ms_slice_to_sleep). This is generally acceptable and matches the behavior of worker
    threads.
  5. Simplified Logic: By delegating the main thread slicing to emscripten_futex_wait, you've removed redundant logic and made __timedwait_cp
    cleaner.

Minor Considerations:

  • Redundant pthread_self() calls: You call pthread_self() twice (once for the state check and once inside the loop for the cancel check).
    While pthread_self() is very fast in Emscripten, you could technically cache it, though this is likely a negligible optimization given the
    other savings.

Overall, the change looks excellent and improves both the code quality and size without regressing functionality.

@sbc100 sbc100 enabled auto-merge (squash) March 20, 2026 23:26
@dschuff
Copy link
Member

dschuff commented Mar 21, 2026

So on the main thread, does this mean that timedwait will slice the sleep to 100ms and futex may slice to 1ms (or 100ms) also?

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 21, 2026

So on the main thread, does this mean that timedwait will slice the sleep to 100ms and futex may slice to 1ms (or 100ms) also?

Yes I believe if you enter __timedwait then and you are cacelable thread (i.e. the normal / default) pthread the sleep will be sliced in 100 intervals here.

In addition, if you are the main runtime thread the futex_wait will then slice that 100ms into 1ms slices.

This should have been part of emscripten-core#26471.

The breaking up of the wait time for 2 of the 3 cases that are handled
here is now handled one layer own in emscripten_futex_wait:

1. Breaking up the wait because we are the main runtime thread.
2. Breaking up the wait because we are async cancelable.

The third cases here (breaking up the wait because we are cancelable
in the non-async sense) still needs to be handled here at the higher
level.
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