Skip to content

Remove PThread.runningWorkers. NFC#26998

Merged
sbc100 merged 1 commit into
emscripten-core:mainfrom
sbc100:remove-running-workers
May 27, 2026
Merged

Remove PThread.runningWorkers. NFC#26998
sbc100 merged 1 commit into
emscripten-core:mainfrom
sbc100:remove-running-workers

Conversation

@sbc100
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 commented May 22, 2026

Remove PThread.runningWorkers and use PThread.pthreads instead to track running workers. PThread.pthreads already contains all the necessary information, and keeping both in sync was redundant.

This also optimizes thread exit by replacing an O(N) array splice with an O(1) map deletion, at the cost of making count queries (mostly used in tests) O(N) instead of O(1).

@sbc100 sbc100 changed the title refactor(pthread): remove PThread.runningWorkers Remove PThread.runningWorkers. NFC May 22, 2026
@sbc100 sbc100 requested review from dschuff and kripken May 22, 2026 23:26
@sbc100 sbc100 enabled auto-merge (squash) May 23, 2026 00:19
@sbc100 sbc100 requested a review from kleisauke May 23, 2026 17:04
@kleisauke
Copy link
Copy Markdown
Collaborator

Although this API is intended for internal use only, I think we need a changelog entry for this. wasm-vips previously relied on this in an EM_JS block (see e.g. commit kleisauke/wasm-vips@1f8497d where it was removed) and I also noticed that emnapi depends on it as well:
https://github.com/search?q=repo%3Atoyobayashi%2Femnapi%20PThread.runningWorkers&type=code

/cc @toyobayashi FYI.

@sbc100 sbc100 force-pushed the remove-running-workers branch from 8f1e710 to bfa8d67 Compare May 24, 2026 16:29
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 24, 2026

Good idea. Added a ChangeLog entry.

Remove `PThread.runningWorkers` and use `PThread.pthreads` instead to
track running workers. `PThread.pthreads` already contains all the
necessary information, and keeping both in sync was redundant.

This also optimizes thread exit by replacing an O(N) array splice with
an O(1) map deletion, at the cost of making count queries (mostly used
in tests) O(N) instead of O(1).

TAG=agy
CONV=47f918e2-bd47-4e38-9a6d-af1765cf2f7e
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 26, 2026

gentle ping @kripken or @kleisauke ?

@sbc100 sbc100 force-pushed the remove-running-workers branch from 4601366 to b0993c4 Compare May 26, 2026 23:28
@kleisauke
Copy link
Copy Markdown
Collaborator

LGTM, but the commit message includes the following trailers:

TAG=agy
CONV=47f918e2-bd47-4e38-9a6d-af1765cf2f7e

I'm not sure whether these were added intentionally or what they are meant to indicate.

@kleisauke
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 merged commit 0c0f3d4 into emscripten-core:main May 27, 2026
30 checks passed
@sbc100 sbc100 deleted the remove-running-workers branch May 27, 2026 08:48
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 27, 2026

LGTM, but the commit message includes the following trailers:

TAG=agy
CONV=47f918e2-bd47-4e38-9a6d-af1765cf2f7e

I'm not sure whether these were added intentionally or what they are meant to indicate.

They were not supposed to be there. Luckily the final commit message used is based on the PR description and not the git commit messages, and they were already removed therre.

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