Skip to content

Conversation

@awtcode
Copy link

@awtcode awtcode commented Dec 11, 2019

This complements the work done in WebAssembly/binaryen#2523 to convert Imports in Dynamic Linking into Indirect Calls. The benefits of doing this are:

  1. Faster performance
  2. Skip legalization
  3. Remove JS stubs (To be done in a later PR)

A sample benchmark has been uploaded here: https://github.com/awtcode/dylink_benchmark

These are the results of the benchmark:

Chrome 78
Original 227ms
PR 17ms
Firefox 71
Original 43ms
PR 6ms

This PR gives a 13x improvement in Chrome and a 7x improvement in FF.

@awtcode
Copy link
Author

awtcode commented Dec 11, 2019

@sbc100 , this is my initial attempt to replace imports with indirect calls. Can you take a look and give me your comments? Thanks.

emscripten.py Outdated
originalAccessor = ''
if shared.Settings.LEGALIZE_JS_FFI and not shared.JS.is_legal_sig(sig):
name = 'orig$' + name
originalAccessor = ('if (!func) func = Module["_%s"];' % (name))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the extra pair of ( ) around this statement.

Copy link
Author

Choose a reason for hiding this comment

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

Sure

emscripten.py Outdated
var func = Module['asm']['%(original)s'];
// If there is no wasm function, this may be a JS library function or
// something from another module.
%(originalAccessor)s
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this part is already handles above now isn't it?

I think that was what alon fixed in #9810? Maybe this part of this change can be reverted now?

Copy link
Author

@awtcode awtcode Dec 12, 2019

Choose a reason for hiding this comment

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

I believe the side exports are added to the Module object and not Module['asm'] thus the line var func = Module['asm']['%(original)s']; can only retrieve the original exports from the Main module and not the side. For this PR, we actually need the original exports from the side module, thus the need to retrieve the original side functions from the Module object instead e.g. originalAccessor = ('if (!func) func = Module["_%s"];' % (name))

emscripten.py Outdated

libraryfile = infile + '.libfile'
with open(libraryfile, 'w') as f:
f.write(str(library_fns_list))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I better format would probably just one line per symbols:

for sym in library_fns_list:
   f.write(sym + '\n')

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we call the file, and this variable somehow more meaningful too? Perhaps "js_imports"? Or "js_symbols"?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, will update the format :)

@awtcode
Copy link
Author

awtcode commented Dec 12, 2019

@sbc100 and @kripken , I have updated the PR with some results from my test app.

@kripken
Copy link
Member

kripken commented Dec 12, 2019

I am uncomfortable with getting the list of JS library functions in this way. It means that there are two kinds of imports, one that is provided by wasm and one by JS, and the module being compiled needs to know which is which at compile time. This feels a little risky.

IIUC this PR makes imported wasm functions be called indirectly. If that is fast enough, then making the JS functions be indirect calls as well may be fast enough too. That is, JS library functions would be added to the table, then treated just like any other import. That layer could also add legalization for JS as needed, which would simplify our legalization story.

(However, that assumes these indirect calls are fast enough, and we may want to keep one direction's calls as direct, for speed? That would make this more complicated.)

@awtcode awtcode force-pushed the convert_imports_to_indirects branch from 6fafe0e to 462cc81 Compare January 21, 2020 07:09
@awtcode awtcode changed the base branch from incoming to master January 21, 2020 07:10
for sym in library_fns_list:
js_symbols_file.write(sym + '\n')
else:
js_symbols = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

I stole this idea/code and made it part of #10350 in a function called get_all_js_library_funcs hopefully that is about to land and we can use that shared function here.

printf("sidey2 says %d", 18);
return 18;
}
''', expected='sidey says 17 sidey2 says 18', need_reverse=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we somehow verify that this worked by looking at import/exports of the resulting wasm file?

@stale
Copy link

stale bot commented Feb 7, 2021

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Feb 7, 2021
Base automatically changed from master to main March 8, 2021 23:49
@stale stale bot removed the wontfix label Mar 8, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Apr 16, 2022
@stale stale bot closed this Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants