Repository navigation
Fix unused and overly-broad __deps entries in JS libraries - #27999
Conversation
9326476 to
181a907
Compare
181a907 to
b055985
Compare
254d97e to
8911a71
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes and cleans up library dependencies across various Emscripten JS library files by removing unused dependencies and conditionalizing others under preprocessor flags, resulting in reduced code sizes. The review feedback correctly identifies an issue in src/lib/libglut.js where glutPositionWindow calls Browser.exitFullscreen() but does not declare $Browser in its dependencies, which could lead to a runtime ReferenceError.
8911a71 to
a71835f
Compare
sbc100
changed the title
__deps entries in JS libraries. NFC__deps entries in JS libraries
|
This is quite large - any suggestions about how to review it? Ideally we'd have some way to automate the check but nothing comes to mind... |
|
I'm sure Gemini could help you review it? You could be sure it say something like "please make sure that you include transitive dependencies in your analysis"? |
|
BTW, this change is really just laying ground work for the much bigger change which simply removes all the JS deps.. so it will be very short lives anyway. |
a71835f to
8fed59e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and optimizes dependency lists across various Emscripten library files by conditionally wrapping them in preprocessor guards or removing unused dependencies, which successfully reduces the compiled code size. The review feedback focuses on improving code readability and maintainability, specifically recommending the use of standard trailing commas instead of fragile leading commas in src/lib/libwebgl.js and wrapping a very long single-line dependency array in src/lib/libhtml5.js onto multiple lines.
994fcc3 to
f7599a0
Compare
|
I split out part of this change: #28034 |
f7599a0 to
e1d9fe9
Compare
Does this make reviewing that one any easier? It has 118 changed files and this one has 19, so the benefit can't be large? Maybe I'm not understanding you, sorry.
That sounds more like Gemini reviewing it 😄 not Gemini helping me. And I am not confident enough in LLMs to land large changes without further verification. It doesn't need to be human verification, if we can find an automatic non-LLM method. If we can't, I can do a human review, but it will take a while. |
Basically its about code size changes. As part of landing #27995 I'm deleting all (most) of the deps entries. Without first cleaning them all up and making them correct then #27995 also ends up including a bunch of code size reductions. I'd like to show that #27995 is a no-op. But unless the deps are cleanup up before i lands then it will not be a no-op WRT to codesize. |
e1d9fe9 to
f62d309
Compare
|
I would like to land this first I think. The PR is now simpler now that #28034 has landed. In terms of tools to help you review we do have |
|
I can split out, for example, the webgl parts. I'll try that now. |
|
webgl changes split out #28037 |
- In `src/lib/libpthread.js`, guard `'$addOnPreRun'` in `$PThread__deps` with `#if !MINIMAL_RUNTIME && PTHREAD_POOL_SIZE`. - Guard or remove other unused and overly-broad `__deps` entries across `src/lib/*.js` (`libasync.js`, `libbrowser.js`, `libdylink.js`, `libemval.js`, `libfetch.js`, `libfs.js`, `libglew.js`, `libglut.js`, `libhtml5.js`, `libnoderawfs.js`, `libsdl.js`, `libsockfs_node.js`, `libwasmfs.js`). Split out from emscripten-core#27995
f62d309 to
ca17fa4
Compare
…es. NFC (#28037) Align the preprocessor guards on `__deps` arrays with the actual call sites in the corresponding function bodies so symbols are only pulled in when referenced: - In `src/lib/libwebgl.js`: - Guard `'$webglBufferSubData'` in `$GL__deps` and `glDrawElements__deps` with `#if FULL_ES2`. In `$GL`, `webglBufferSubData` is only called from `preDrawHandleClientVertexAttribBindings` (which is guarded by `#if FULL_ES2`, not `LEGACY_GL_EMULATION`). Similarly, `glDrawElements` only calls `webglBufferSubData` inside its `#if FULL_ES2` client-side index buffer block. - Guard `'$heapObjectForWebGLType'` and `'$toTypedArrayIndex'` in `glTexImage2D__deps`, `glTexSubImage2D__deps`, and `glReadPixels__deps` with `#if WEBGL_USE_GARBAGE_FREE_APIS` instead of `#if MAX_WEBGL_VERSION >= 2`. Those functions only call `heapObjectForWebGLType` and `toTypedArrayIndex` directly inside `#if WEBGL_USE_GARBAGE_FREE_APIS` blocks; otherwise they delegate to `emscriptenWebGLGetTexPixelData` (which declares its own `__deps`). - Guard `$webglGetProgramUniformLocation__deps` with `#if GL_EXPLICIT_UNIFORM_LOCATION`, matching the sole call to `webglPrepareUniformLocationsBeforeFirstUse` inside `$webglGetProgramUniformLocation`. - In `src/lib/libwebgl2.js`: - Guard `'$toTypedArrayIndex'` in `glTexImage3D__deps` and `glTexSubImage3D__deps` with `#if WEBGL_USE_GARBAGE_FREE_APIS`, since `toTypedArrayIndex` is only called in that branch (the `#else` branch delegates to `emscriptenWebGLGetTexPixelData`). - Remove `glGetVertexAttribIuiv__deps`. `glGetVertexAttribIuiv` is a string alias for `'glGetVertexAttribIiv'`, which already declares `glGetVertexAttribIiv__deps: ['$emscriptenWebGLGetVertexAttrib']`. Split out from #27999
…FC (#28034) In `src/lib/libcore.js`, check `t.includes('SYSCALLS.')` instead of `t.includes('SYSCALLS')` in `wrapSyscallFunction` so comments and strings mentioning `SYSCALLS_REQUIRE_FILESYSTEM` (such as in `fd_write`) do not pull in `$SYSCALLS` and `$UTF8ToString` when `SYSCALLS_REQUIRE_FILESYSTEM=0`. Split out from #27999
src/lib/libpthread.js, guard'$addOnPreRun'in$PThread__depswith
#if !MINIMAL_RUNTIME && PTHREAD_POOL_SIZE.__depsentries acrosssrc/lib/*.js(libasync.js,libbrowser.js,libdylink.js,libemval.js,libfetch.js,libfs.js,libglew.js,libglut.js,libhtml5.js,libnoderawfs.js,libsdl.js,libsockfs_node.js,libwasmfs.js,libwebgl.js,libwebgl2.js).Split out from #27995