Use normal JS library aliases for GL symbols. NFC#19033
Merged
Conversation
The `recordGLProcAddressGet` helper was attempt to resolve and create aliases using `copyLibEntry`, but this function doesn't work when the target of the alias exists in a different library file. This was resulting `undefined` being set for these aliases and the resulting JS code would contains, for example: ``` var glGenFramebuffersOES = undefined; ``` Completely removing the aliasing resolution and the symbol copying from `recordGLProcAddressGet` seems to do the right thing, and should save on code size too: ``` var _glGenFramebuffersOES = _glGenFramebuffers; ```
kripken
reviewed
Mar 22, 2023
Member
kripken
left a comment
There was a problem hiding this comment.
What is the difference in the output from this? (I forget, does the normal alias mechanism duplicate the function or not? The removed code looks like it duplicates.)
Collaborator
Author
Yes, it removes the duplication, so it should be codesize saving too. I think the duplication might have been there from the fastcomp days when the JS and native symbol space was more like a single thing? I'm not sure. |
Member
|
It's possible that back then we thought it might be slightly faster to duplicate the code. But I doubt that's true. |
kripken
approved these changes
Mar 22, 2023
sbc100
added a commit
that referenced
this pull request
Mar 22, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier chooses not to include the JS version but the location where the aliases are declared in the output comes before the native symbols are declared. To fix this I removed the native aliases from webgl2.c, since these exact same aliases already exist in library_webgl.js. This think is also more correct since it means that the `emscripten_xx` wrappers are defined fully in JS and are not effected by definitions in native code (which is the whole point of them I believe).
sbc100
added a commit
that referenced
this pull request
Mar 22, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier chooses not to include the JS version but the location where the aliases are declared in the output comes before the native symbols are declared. To fix this I removed the native aliases from webgl2.c, since these exact same aliases already exist in library_webgl.js. This think is also more correct since it means that the `emscripten_xx` wrappers are defined fully in JS and are not effected by definitions in native code (which is the whole point of them I believe).
sbc100
added a commit
that referenced
this pull request
Mar 22, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier chooses not to include the JS version but the location where the aliases are declared in the output comes before the native symbols are declared. To fix this I removed the native aliases from webgl2.c, since these exact same aliases already exist in library_webgl.js. This think is also more correct since it means that the `emscripten_xx` wrappers are defined fully in JS and are not effected by definitions in native code (which is the whole point of them I believe).
sbc100
added a commit
that referenced
this pull request
Mar 22, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier chooses not to include the JS version but the location where the aliases are declared in the output comes before the native symbols are declared. To fix this I removed the native aliases from webgl2.c, since these exact same aliases already exist in library_webgl.js. This think is also more correct since it means that the `emscripten_xx` wrappers are defined fully in JS and are not effected by definitions in native code (which is the whole point of them I believe).
sbc100
added a commit
that referenced
this pull request
Mar 22, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier chooses not to include the JS version but the location where the aliases are declared in the output comes before the native symbols are declared. To fix this I removed the native aliases from webgl2.c, since these exact same aliases already exist in library_webgl.js. This think is also more correct since it means that the `emscripten_xx` wrappers are defined fully in JS and are not effected by definitions in native code (which is the whole point of them I believe).
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 23, 2023
Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
sbc100
added a commit
that referenced
this pull request
Mar 30, 2023
#19046) Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
recordGLProcAddressGethelper was attempt to resolve and create aliases usingcopyLibEntry, but this function doesn't work when the target of the alias exists in a different library file.This was resulting
undefinedbeing set for these aliases and the resulting JS code would contains, for example:Completely removing the aliasing resolution and the symbol copying from
recordGLProcAddressGetseems to do the right thing, and should save on code size too: