Skip to content

Fix performance issue with WebGL 2 glBufferData #15206

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Matheus28
Copy link

Turns out the spec is dumb and considers size = 0 as "the rest of the buffer", meaning it would try to copy the entire heap if data isn't null. This is easily solved by checking for this condition and using a null data if size is zero.

Unfortunately for bufferSubData, passing just (target, offset, size) doesn't work, so we need to fall back to the method that allocates (can be easily solved with a global size 0 arraybuffer, but I don't know the internals of the emscripten js libraries enough to do it myself quickly)

…a != null

Turns out the spec is dumb and considers size = 0 as "the rest of the buffer", meaning it would try to copy the entire heap if data isn't null. Unfortunately for bufferSubData, passing just (target, offset, size) doesn't work, so we fall back to the method that allocates (can be easily solved with a global size 0 arraybuffer)
@kripken kripken requested review from juj and kainino0x October 4, 2021 16:38
@@ -1767,7 +1767,7 @@ var LibraryGL = {

#if MAX_WEBGL_VERSION >= 2
if ({{{ isCurrentContextWebGL2() }}}) { // WebGL 2 provides new garbage-free entry points to call to WebGL. Use those always when possible.
if (data) {
if (data && size > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add at the top a

#if ASSERTIONS
  assert(size >= 0);
#endif

and then here do just if (data && size).

We do not currently support larger than 2GB buffers (that needs special handling, which I doubt anyone will use with WebGL - those use cases will be with Wasm64 and WebGPU)

@@ -1785,7 +1785,7 @@ var LibraryGL = {
glBufferSubData__sig: 'viiii',
glBufferSubData: function(target, offset, size, data) {
#if MAX_WEBGL_VERSION >= 2
if ({{{ isCurrentContextWebGL2() }}}) { // WebGL 2 provides new garbage-free entry points to call to WebGL. Use those always when possible.
if ({{{ isCurrentContextWebGL2() }}} && size > 0) { // WebGL 2 provides new garbage-free entry points to call to WebGL. Use those always when possible.
GLctx.bufferSubData(target, offset, HEAPU8, data, size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would lead to calling the WebGL 1 style function below if size <= 0.

How about the following?

  glBufferSubData: function(target, offset, size, data) {
#if ASSERTIONS
  assert(size >= 0);
#endif

#if MAX_WEBGL_VERSION >= 2
     if ({{{ isCurrentContextWebGL2() }}}) { // WebGL 2 provides new garbage-free entry points to call to WebGL. Use those always when possible.
       size && GLctx.bufferSubData(target, offset, HEAPU8, data, size);
       return;
     }
#endif
    GLctx.bufferSubData(target, offset, HEAPU8.subarray(data, data+size));
  },

Copy link
Author

@Matheus28 Matheus28 Oct 5, 2021

Choose a reason for hiding this comment

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

I was under the impression that bufferSubData could affect something in the buffer state, that's why I was letting it fall back to the webgl 1 style function, but since it doesn't affect anything... Might as well move the size check to also include webgl 1 to avoid the allocation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might as well move the size check to also include webgl 1 to avoid the allocation?

That does make sense.. shortest code is probably to also do

size && GLctx.bufferSubData(target, offset, HEAPU8.subarray(data, data+size));

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, actually, although generally it is preferable to "diligently" call the functions so that WebGL spec related validation and GL error state etc. is properly recorded, so in the absence of other functional difference, may be good to leave that out in the WebGL 1 path. That way the target and currently bound buffer etc. will be properly validated.

@stale
Copy link

stale bot commented Nov 2, 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 Nov 2, 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.

2 participants