Skip to content
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

Append em-pthread id to the web worker name. #22644

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

kolAflash
Copy link
Contributor

Simplifies picking a web worker when using browser debug tools.

@kolAflash
Copy link
Contributor Author

Currently all web workers created for threads are called "em-pthread". So when using browser debug tools, you have to try out one web worker after another until you find the one you'd like to debug.

@@ -405,7 +405,7 @@ var LibraryPThread = {
#if ENVIRONMENT_MAY_BE_WEB || ENVIRONMENT_MAY_BE_WORKER
// This is the way that we signal to the Web Worker that it is hosting
// a pthread.
'name': 'em-pthread',
'name': 'em-pthread_' + PThread.nextWorkerID,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use a hyphen here?

@sbc100
Copy link
Collaborator

sbc100 commented Sep 30, 2024

It looks like closure does't like this:

building:ERROR: /tmp/emscripten_temp_qawuf4_3/test.jso4.js:356:29: WARNING - [JSC_INEXISTENT_PROPERTY] Property nextWorkerID never defined on PThread
  356| "name":"em-pthread-"+PThread.nextWorkerID};var pthreadMainJs=_scriptName;// We can't use makeModuleReceiveWithVar here since we want to also

@sbc100
Copy link
Collaborator

sbc100 commented Sep 30, 2024

Ah yes, nextWorkerID is debug-only feature.

@kolAflash
Copy link
Contributor Author

kolAflash commented Oct 4, 2024

@sbc100

Ah yes, nextWorkerID is debug-only feature.

Thanks for the finding!
Any suggestions how to resolve this?

I'm not well familiar with the Emscripten code base.
Would it be an option to make nextWorkerID available in non-debug mode?

I've pushed a commit where I add a new variable workerId for counting the workers. But I guess that's not the optimal solution.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 7, 2024

I would rather you use the existing nextWorkerID, but only add the -N suffix when ASSERTIONS are enabled.

Simplifies picking a web worker when using browser debug tools.
@kolAflash
Copy link
Contributor Author

I'd prefer to always have worker thread IDs. Without IDs debugging is just pure pain.

But I've just pushed a commit which puts #if ASSERTIONS around it. And compared to how it was before it's an improvement.

Note:
As far as I understand, this can be enabled by using emscripten with the LDFLAG -s ASSERTIONS=1.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 8, 2024

I'd prefer to always have worker thread IDs. Without IDs debugging is just pure pain.

But I've just pushed a commit which puts #if ASSERTIONS around it. And compared to how it was before it's an improvement.

Note: As far as I understand, this can be enabled by using emscripten with the LDFLAG -s ASSERTIONS=1.

Exactly. Also any build with -O0 (which is also the default) will have assertions enabled. In other words, only release builds won't show these IDs.

Also, just to be clear, you are aware there this is not actually a pthread ID but a worker ID, and a single worker and be re-used to hold different pthreads since they get reused once a pthread exits.

@sbc100 sbc100 enabled auto-merge (squash) October 8, 2024 16:05
@sbc100 sbc100 merged commit f530ed0 into emscripten-core:main Oct 8, 2024
28 checks passed
@kolAflash
Copy link
Contributor Author

[...] Also any build with -O0 (which is also the default) will have assertions enabled. In other words, only release builds won't show these IDs.

Good to know.

Also, just to be clear, you are aware there this is not actually a pthread ID but a worker ID, and a single worker and be re-used to hold different pthreads since they get reused once a pthread exits.

I know the difference, even though I sometimes miss to distinguish between C / C++ thread and web worker.

I didn't know that emscripten reuses a worker when it's assigned thread dies. But debugging short lived threads is anyway always a little harder. And for long living threads the merged solution is very helpful I think.

 

Thanks!

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