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

Initial support for pthreads + dynamic linking #13245

Merged
merged 1 commit into from
Jan 14, 2021

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 13, 2021

This support is still experimental and has some major
caveats:

  • Only supports load-time dynamic linking
  • No support for sharing TLS data between dylibs.

The major change is that workers now also call the module run function
although they exit early once dynamic libraries have all been loaded.

Another change is the dynamic library loading (which needs to run on
each new worker) is now seperated from __ATPRERUN__ (which only wants
to run once on the main thread).

See: #3494

@sbc100 sbc100 force-pushed the pthreads_dynamic_linking2 branch from 3594754 to ba8e0d5 Compare January 13, 2021 12:58
sbc100 added a commit that referenced this pull request Jan 13, 2021
sbc100 added a commit that referenced this pull request Jan 13, 2021
@sbc100 sbc100 force-pushed the pthreads_dynamic_linking2 branch from ba8e0d5 to 424ad10 Compare January 13, 2021 13:11
@sbc100 sbc100 changed the base branch from master to refactor_stack_check_init January 13, 2021 13:11
Base automatically changed from refactor_stack_check_init to master January 13, 2021 14:06
sbc100 added a commit that referenced this pull request Jan 13, 2021
@sbc100 sbc100 force-pushed the pthreads_dynamic_linking2 branch from 424ad10 to 39b4e05 Compare January 13, 2021 14:09
@sbc100 sbc100 requested review from kripken and dschuff January 13, 2021 14:23
@sbc100 sbc100 force-pushed the pthreads_dynamic_linking2 branch 6 times, most recently from 4c5afd5 to 973f7bc Compare January 13, 2021 16:00
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.

Impressively small!

@sbc100 sbc100 force-pushed the pthreads_dynamic_linking2 branch from 973f7bc to a571fd1 Compare January 13, 2021 17:43
sbc100 added a commit that referenced this pull request Jan 13, 2021
This avoids running this code than once in the case that
run() is called several times during startup which happens when
run dependencies are injected.

Split out from #13245
@sbc100 sbc100 force-pushed the pthreads_dynamic_linking2 branch from a571fd1 to 07ae0b0 Compare January 13, 2021 18:06
@sbc100 sbc100 force-pushed the pthreads_dynamic_linking2 branch from 07ae0b0 to 3e588ab Compare January 13, 2021 18:08
sbc100 added a commit that referenced this pull request Jan 13, 2021
This avoids running this code than once in the case that
run() is called several times during startup which happens when
run dependencies are injected.

Split out from #13245
sbc100 added a commit that referenced this pull request Jan 13, 2021
This avoids running this code than once in the case that
run() is called several times during startup which happens when
run dependencies are injected.

Split out from #13245
@sbc100 sbc100 force-pushed the pthreads_dynamic_linking2 branch from 3e588ab to e7aa081 Compare January 13, 2021 23:32
@sbc100 sbc100 changed the base branch from master to start_check_init January 14, 2021 09:36
@sbc100 sbc100 force-pushed the pthreads_dynamic_linking2 branch 2 times, most recently from c0ba508 to 7f21f38 Compare January 14, 2021 11:04
@sbc100 sbc100 changed the base branch from start_check_init to master January 14, 2021 11:09
@sbc100 sbc100 force-pushed the pthreads_dynamic_linking2 branch from 7f21f38 to d927be3 Compare January 14, 2021 14:32
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.

Thanks, makes sense. lgtm.

Comment on lines +271 to +272
// Loading of dynamic libraries needs to happen on each thread, so we can't
// use the normal __ATPRERUN__ mechanism.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Loading of dynamic libraries needs to happen on each thread, so we can't
// use the normal __ATPRERUN__ mechanism.
// Loading of dynamic libraries needs to happen on each thread, so we can't
// use the normal __ATPRERUN__ mechanism.

#if MAIN_MODULE
preloadDylibs();
#else
reportUndefinedSymbols();
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, as it's pre-existing, but do you know why the main thread loads dylibs while the side module reports undefined symbols? That seems odd. I'd expect the main module to do both?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait, the side module has no JS. So basically, if it's relocatable but not a main module, it only reports undefined symbols, I guess?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed. Although I'm not really sure how much that configuration gets used .. I don't think we test it much.

This support is still experimental and has some major
caveats:

- Only supports load-time dynamic linking
- No support for sharing TLS data between dylibs.

The major change is that workers now also call the module `run` function
although they exit realy once dynamic libraries have all been loaded.

Another change is the dynamic library loading (which needs to run on
each new worker) is now seperated from `__ATPRERUN__` (which only wants
to run once on the main thread).
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