Skip to content

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Nov 29, 2020

The main thread pointer is already available via the
_emscripten_main_browser_thread_id native
function.

There is no point in also storing it in JS.

While making this change I noticed that postamble_minimal.js
was also redundantly calling __emscripten_thread_init and
_emscripten_register_main_browser_thread_id.

@sbc100 sbc100 requested review from juj and kripken November 29, 2020 19:45
@sbc100 sbc100 force-pushed the remove_mainThreadBlock branch from 0d4fbf9 to 440d320 Compare November 29, 2020 20:40
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

This looks correct, but it means JS calls into wasm to get the main thread ID. That may be slower, so we should check if none of the calls are on an important code path?

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 30, 2020

This looks correct, but it means JS calls into wasm to get the main thread ID. That may be slower, so we should check if none of the calls are on an important code path?

Yes I checked that. Should have mentioned in the PR.

The only two call sites that needed to be updated were from _emscripten_do_pthread_join and from threadProfiler.updateUi.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

I see. In theory join is maybe called in a busy place somewhere, but it seems unlikely, I gree.

The main thread pointer is already available via the
`_emscripten_main_browser_thread_id` native
function.

There is no point in also storing it in JS.

While making this change I noticed that postamble_minimal.js
was also redundantly calling `__emscripten_thread_init` and
`_emscripten_register_main_browser_thread_id`.
@sbc100 sbc100 force-pushed the remove_mainThreadBlock branch from 440d320 to 4e5676b Compare December 1, 2020 01:55
@sbc100 sbc100 merged commit 45aa70c into master Dec 1, 2020
@sbc100 sbc100 deleted the remove_mainThreadBlock branch December 1, 2020 03:54
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