Skip to content

Remove timeout slicing from __timedwait. NFC#26550

Merged
sbc100 merged 1 commit intoemscripten-core:mainfrom
sbc100:__timedwait_part2
Mar 26, 2026
Merged

Remove timeout slicing from __timedwait. NFC#26550
sbc100 merged 1 commit intoemscripten-core:mainfrom
sbc100:__timedwait_part2

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Mar 26, 2026

This removes the timeout slicing from __timedwait and instead relies
on the existing slicing in emscripten_futex_wait.c.

This is a followup to #26511 and #26471 (which did the same for the
slicing in __wait).

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 26, 2026

This change is currently stacked on top of #26549

@sbc100 sbc100 force-pushed the __timedwait_part2 branch from 93ec16e to 0050751 Compare March 26, 2026 00:24
@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 26, 2026

From Gemini:

✦ Review of Commit 0050751: "Remove timeout slicing from __timedwait. NFC"

This commit simplifies the Emscripten-specific wait logic by centralizing timeout slicing in emscripten_futex_wait.c.

  1. system/lib/libc/musl/src/thread/__timedwait.c
  • Change: Removed the manual slicing loop (max_ms_slice_to_sleep) and the cancellation check within __timedwait_cp. It now directly calls emscripten_futex_wait.
  • Assessment: High-quality simplification. By delegating the slicing and cancellation handling to the lower-level emscripten_futex_wait, we reduce code duplication and complexity in the musl-specific
    part of the library.
  • Naming: Changed msecsToSleep (camelCase) to msecs_to_sleep (snake_case), which is more consistent with musl/libc style.
  1. system/lib/pthread/emscripten_futex_wait.c
  • Change: Updated cancelable calculation to self->canceldisable != PTHREAD_CANCEL_DISABLE.
  • Assessment: This is a critical change. It ensures that any thread that hasn't explicitly disabled cancellation is considered "cancelable" for the purpose of the slicing loop. This correctly includes
    PTHREAD_CANCEL_MASKED (used by pthread_cond_wait), which is essential for correct condition variable behavior.
  • Refinement: Replaced a simple break with a more robust error return:

1 emscripten_conditional_set_current_thread_status(EM_THREAD_STATUS_WAITFUTEX, EM_THREAD_STATUS_RUNNING);
2 return -ECANCELED;
This ensures the thread status is correctly restored before returning the cancellation error to the caller (e.g., __timedwait_cp).

  1. Code Size (test/codesize/*.json)
  • Change: Significant reduction in WASM size (~200 bytes).
  • Assessment: Excellent. Centralizing logic often leads to smaller binaries in Emscripten, and this is a clear win. The reordering of imports (exit vs emscripten_get_now) is a side effect of the symbol
    usage changes but is harmless.
  1. Structural Consistency

Final Verdict: LGTM
The change is a clean, "No Functional Change" (NFC) refactor that improves maintainability and reduces code size without altering the intended behavior (now that async cancellation has been correctly
reverted in the previous commit).

This removes the timeout slicing from `__timedwait` and instead relies
on the existing slicing in `emscripten_futex_wait.c`.

This is a followup to emscripten-core#26511 and emscripten-core#26471 (which did the same for the
slicing in `__wait`).
@sbc100 sbc100 force-pushed the __timedwait_part2 branch from 0050751 to 8c98aef Compare March 26, 2026 21:32
@sbc100 sbc100 requested review from dschuff and kripken March 26, 2026 21:36
@sbc100 sbc100 merged commit ee036b7 into emscripten-core:main Mar 26, 2026
38 checks passed
@sbc100 sbc100 deleted the __timedwait_part2 branch March 26, 2026 22:31
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