Fix a crash in miniTempWebGL*Buffers optimization#22130
Merged
kainino0x merged 2 commits intoemscripten-core:mainfrom Jun 24, 2024
Merged
Fix a crash in miniTempWebGL*Buffers optimization#22130kainino0x merged 2 commits intoemscripten-core:mainfrom
kainino0x merged 2 commits intoemscripten-core:mainfrom
Conversation
library_webgl.js attempts to use a pool of temp float or int buffers for uniform uploads up to 288 elements, however it would only allocate 288 temp buffers, so 0 through 287 would be fine but 288 would crash (miniTempWebGLFloatBuffers[288] would be undefined). Fix for emscripten-core#21573
sbc100
reviewed
Jun 22, 2024
| $miniTempWebGLFloatBuffers__postset: `var miniTempWebGLFloatBuffersStorage = new Float32Array({{{ GL_POOL_TEMP_BUFFERS_SIZE }}}); | ||
| for (/**@suppress{duplicate}*/var i = 0; i < {{{ GL_POOL_TEMP_BUFFERS_SIZE }}}; ++i) { | ||
| // Create GL_POOL_TEMP_BUFFERS_SIZE+1 temporary buffers, for uploads of size 0 through GL_POOL_TEMP_BUFFERS_SIZE inclusive | ||
| for (/**@suppress{duplicate}*/var i = 0; i <= {{{ GL_POOL_TEMP_BUFFERS_SIZE }}}; ++i) { |
Collaborator
There was a problem hiding this comment.
I guess I was the one who broke this in #21573?
Could we instead update all the other places where GL_POOL_TEMP_BUFFERS_SIZE to use less than? e.g. all the places there we currently do if (count <= {{{ GL_POOL_TEMP_BUFFERS_SIZE }}})
Collaborator
Author
There was a problem hiding this comment.
I considered this but decided it didn't make as much sense. Then the top end would be unused for vec2/vec3/vec4 - the maximum values that hit the optimization would be:
- 287 floats
- 143 vec2s (286 floats)
- 95 vec3s (285 floats)
- 71 vec4s (284 floats)
Also
miniTempWebGLFloatBuffersStorage = new Float32Array({{{ GL_POOL_TEMP_BUFFERS_SIZE }}});
would change to
miniTempWebGLFloatBuffersStorage = new Float32Array({{{ GL_POOL_TEMP_BUFFERS_SIZE }}} - 1);
sbc100
approved these changes
Jun 24, 2024
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.
library_webgl.js attempts to use a pool of temp float or int buffers for uniform uploads up to 288 elements, however it would only allocate 288 temp buffers, so 0 through 287 would be fine but 288 would crash (miniTempWebGLFloatBuffers[288] would be undefined).
Confirmed manually that any one of the six "Just at the optimization limit" calls would crash before, and with the fix does not.
Fix for the change in PR #21573