Skip to content

Use wasmImports as the single global symbol table for dynamic linking#18540

Merged
sbc100 merged 1 commit into
mainfrom
unify_symbol_table
Jan 25, 2023
Merged

Use wasmImports as the single global symbol table for dynamic linking#18540
sbc100 merged 1 commit into
mainfrom
unify_symbol_table

Conversation

@sbc100

@sbc100 sbc100 commented Jan 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Previously we were looking up symbols on the Module object which meant building with EXPORT_ALL and exporting all symbols there. This change adds all needed symbols to the wasmImports map and uses that as the single source for symbol lookups.

This is split out from my work on #18376.

This should be a code size win for most users of dynamic linking. It also avoids extra symbols being present on the Module object that the user didn't ask for.

@sbc100
sbc100 force-pushed the unify_symbol_table branch 3 times, most recently from df7040b to 7af37d7 Compare January 18, 2023 03:16
@sbc100 sbc100 changed the title Use asmLibraryArg as the single global symbol table for dynamic linking Use wasmImports as the single global symbol table for dynamic linking Jan 18, 2023
@sbc100
sbc100 force-pushed the unify_symbol_table branch 5 times, most recently from 6ca3011 to 2794cfa Compare January 23, 2023 21:49
@sbc100
sbc100 requested a review from kripken January 23, 2023 22:30
@sbc100
sbc100 force-pushed the unify_symbol_table branch 2 times, most recently from 5a022f5 to 51cce5d Compare January 24, 2023 00:23
kripken
kripken reviewed Jan 24, 2023
View reviewed changes
Comment thread emscripten.py Outdated
Comment thread src/library_dylink.js Outdated
Comment thread src/jsifier.js Outdated
Comment thread src/library_exceptions.js Outdated
sbc100 added a commit that referenced this pull request Jan 24, 2023
@sbc100
Remove unneeded exportedAsmFunc helper. NFC
cc1d3d3 
It looks like this was originally added back in #8040, but was likely
not needed.

At least in of the of three remaining places this function is used
we don't need to go via the `Module` object and these symbols can/should
simply be referenced via the current scope.

Split out from #18540
@sbc100 sbc100 mentioned this pull request Jan 24, 2023
Merged
sbc100 added a commit that referenced this pull request Jan 24, 2023
@sbc100
Remove unneeded exportedAsmFunc helper. NFC (#18588)
5c756de 
It looks like this was originally added back in #8040, but was likely
not needed.

At least in each the of three remaining places this function is used
we don't need to go via the `Module` object and these symbols can/should
simply be referenced via the current scope.

Split out from #18540
@sbc100
sbc100 force-pushed the unify_symbol_table branch 3 times, most recently from 41fb296 to f5b3266 Compare January 25, 2023 00:43
@sbc100
sbc100 requested a review from kripken January 25, 2023 00:43
@sbc100
sbc100 force-pushed the unify_symbol_table branch from f5b3266 to 9d2f486 Compare January 25, 2023 01:06
kripken
kripken reviewed Jan 25, 2023
View reviewed changes
Comment thread src/preamble.js Outdated
Comment thread src/jsifier.js
kripken
kripken reviewed Jan 25, 2023
View reviewed changes
Comment thread src/jsifier.js Outdated
Comment thread src/jsifier.js
Comment thread src/library_dylink.js Outdated
Comment thread src/library_dylink.js Outdated
Comment thread src/library_dylink.js Outdated
Comment thread src/preamble.js Outdated
Comment thread src/preamble.js Outdated
// wasmImports for as-yet-undefined symbols doesn't work since ASYNCIFY then
// wraps these stub functions and we can't then replace them directly. Instead
// the stub functions call into `asyncifyStubs` which gets populated by the
// dyanmic linker and symbols are loaded.

@kripken kripken Jan 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Which functions end up here? I thought all of them, initially, but reading other code I am not sure.

@kripken kripken Jan 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That is, one place seems to always read from asyncifyStubs, but another place checks if a thing is there or not, so I am confused.

@sbc100 sbc100 Jan 25, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Only functions for which we create stubs need to appear in this map. Its the same set of functions that get the .stub attribute set up them. These are functions which are required to instantiate the main module, but which are provided later, by a side module.

Without ASYNCIFY we simply replace the existing .stub function wasmImports once the symbols gets resolved.
With ASYNCIFY this appraoch doesn't work because the thing in the wasmImports dict might be an asyncify wrapper around the .stub we created. In this case we replace the entry in asyncifyStubs which is called by the stub function.

@sbc100 sbc100 Jan 25, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There are two places the asyncifyStubs are used.

  1. In the stub functions themselves. In debug builds we assert that that right entry is present. In release builds we will just trap if its not present.
  2. In the dynamic linker: When a new symbol is loaded it replaces the entry in asyncifyStubs, but only if that key already exists in the dictionary.

@sbc100
Use wasmImports as the single global symbol table for dynamic linking
f7e4555 
Previously we were looking up symbols on the `Module` object which
meant building with `EXPORT_ALL` and exporting all symbols there.  This
change adds all needed symbols to the `wasmImports` maps and uses that
as the single source for symbol lookups.

This is split out from my work on #18376.

This should be a code size win in for most users of dynamic linking.  It
also avoids extra symbols being present on the `Module` object that the
user didn't ask for.
@sbc100
sbc100 force-pushed the unify_symbol_table branch from 9d2f486 to f7e4555 Compare January 25, 2023 18:04
kripken
kripken approved these changes Jan 25, 2023
View reviewed changes

@kripken kripken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, that's what I was missing. Code makes sense to me now...

Comment thread tools/building.py
@sbc100
sbc100 merged commit 612577a into main Jan 25, 2023
@sbc100
sbc100 deleted the unify_symbol_table branch January 25, 2023 21:30
@sbc100 sbc100 mentioned this pull request Mar 5, 2023
Closed
@wenhanchong wenhanchong mentioned this pull request Oct 11, 2023
Open
@olokobayusuf olokobayusuf mentioned this pull request Mar 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kripken kripken kripken approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sbc100 @kripken