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.

Sign up for GitHub

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
Open

Fix performance issue with WebGL 2 glBufferData #15206

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/library_webgl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

GLctx.bufferData(target, HEAPU8, usage, data, size);
} else {
GLctx.bufferData(target, size, usage);
Expand All @@ -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 •
edited
Loading

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.

return;
}
Expand Down