-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add new simple export mechanism #22977
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
base: main
Are you sure you want to change the base?
Conversation
@brendandahl FYI still WIP |
e5e4d41
to
0cb2571
Compare
This change adds a new `-sEXPORT` setting that can by used to export any type of symbols (see emscripten-core#8380) It also includes a new `-sMANGLED_SYMBOLS` setting which default to true in order to not break backwards compatibility. Both these new settings are currently experimental and using `-sEXPORT` currently disables `-sMANGLED_SYMBOLS` by default.
0cb2571
to
3558467
Compare
@@ -26,7 +26,7 @@ Module['thisProgram'] = 'thisProgram'; // for consistency between different buil | |||
|
|||
function hashMemory(id) { | |||
var ret = 0; | |||
var len = _sbrk(0); | |||
var len = sbrk(0); |
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.
How does this work? I was thinking that MANGLED_SYMBOLS
controls whether we have _sbrk
or sbrk
, so how can this code have only one of the two forms?
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.
The idea is that internally the unmangled symbols are always available and ever present.
If somebody wants unmangled symbols they are generated in addition. I'm hoping DCE should take care the most of the cost of this.
If we didn't do it this way then all of these internal call sites would somehow have to be conditional, which seem like a bad idea.
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 see. Yeah, that makes sense. If we verify that JS optimizations remove the duplicate, then sgtm.
}, | ||
h: function() { | ||
return n | 0; | ||
} |
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.
These three new functions are maybe worth looking into. The first and last should be getting inlined, surely.
Perhaps we can add a tiny Acorn pass to deduplicate the mangled/unmangled globals, if they are the cause of this, and if Closure is not good enough to do it itself.
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 skimmed the critical parts. Looks very promising! Great to see this happening.
Looks good so far! I imagine this will bitrot very quickly. I'm happy to look into failures next week if you want help. |
|
This change adds a new
-sEXPORT
setting that can by used to export any type of symbols (see #8380)It also includes a new
-sMANGLED_SYMBOLS
setting which default to true in order to not break backwards compatibility.Both these new settings are currently experimental and using
-sEXPORT
currently disables-sMANGLED_SYMBOLS
by default.