-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Clean up worker.js a bit #9018
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
Clean up worker.js a bit #9018
Conversation
Removed unused functions/vars, terminated unterminated statements,
|
For some reason after changing a const to a var I'm getting a bunch of assertion failures like this: |
|
Maybe you need to rebase. The expected version was recently updated. |
| var threadInfoStruct = 0; // Info area for this thread in Emscripten HEAP (shared). If zero, this worker is not currently hosting an executing pthread. | ||
| var selfThreadId = 0; // The ID of this thread. 0 if not hosting a pthread. | ||
| var parentThreadId = 0; // The ID of the parent pthread that launched this thread. | ||
| var tempDoublePtr = 0; // A temporary memory area for global float and double marshalling operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is used on line 90, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, that's Module['tempDoublePtr'] and e.data.tempDoublePtr.
Unless I'm missing something, neither of those are the tempDoublePtr defined in the worker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, line 90 has {{{ makeAsmGlobalAccessInPthread('tempDoublePtr') }}} = e.data.tempDoublePtr;, and that will turn into either
tempDoublePtr = e.data.tempDoublePtr;
or if MODULARIZE,
Module['tempDoublePtr')] = e.data.tempDoublePtr;
The former case does reference that global.
But thinking more, it looks like maybe it shouldn't, though, if that's the only reference - nothing sets it. Thinking yet more, this is not going to matter with the upstream backend anyhow (which doesn't use that special global).
Anyhow, the change as is will write to a JS global (if not modularize), after removing the var, which I think we should avoid as it would be a regression. Probably that line should always write to Module['tempDoublePtr'] (avoiding makeAsmGlobalAccessInPthread), unless maybe the line should only execute in modularize mode, but that sound wrong to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recently changed this in #9026. You might want to merge in new changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. Sounds like tempDoublePtr should only be available under !WASM_BACKEND && !MODULARIZE.
Thanks quantum5, I've merged your changes in.
|
Failures are now: test_glgears_long, test_sdl2_ttf, and the doc tests. Pretty sure none of those are me? |
|
Looks good, thanks @VirtualTim! |
Removed unused functions/vars, terminated unterminated statements, * Changed const to var * Change "this.err" to "var err" * tempDoublePtr should only be available in !MODULARIZE
Removed unused functions/vars, terminated unterminated statements,
I'm unsure what the deal is with
PThread.thisThreadCancelState, since I can't find any reference tothisThreadCancelStatein the Emscripten source. Maybe this is always undefined? Should it beC_STRUCTS.pthread.threadExitCode?