973810 - Extend WebGL to not require creating one-off ArrayBufferViews to prevent garbage creation

Status: RESOLVED DUPLICATE of bug 1313541

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140214030202

Comment 1

[u480271]

Vlad requested via IRC and extension to WebGL to take ArrayBuffer, offset and length to bypass the creation on short-lived, GC'd View objects in emscripten generated code.

Comment 2

[u480271]

Attachment: MOZ_WEBGL_array_buffer_offset_length

Here's a quick hack implementing Vlad's request. There is so much open for
discussion, #1 being the name is horrible.

Comment 3

[u480271]

Attachment: bug-973810-MOZ_WEBGL_array_buffer_offset_length.patch

Merged my previous patch with Vlad's from github.

Comment 4

[u480271]

Attachment: library_gl.js.patch

Here is how I *hacked* support into emscripten. I haven't tested this yet.

Comment 5

[Masatoshi Kimura [:emk]]

Weren't prefixed extensions deprecated? (bug 924176)

Comment 6

[u480271]

I believe I removed the MOZ_ prefix, or am I missing something?

Comment 7

[Masatoshi Kimura [:emk]]

Oh, I was confused by the first obsolete patch, sorry.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8380420 [details] [diff] [review]
bug-973810-MOZ_WEBGL_array_buffer_offset_length.patch

Review of attachment 8380420 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContext.h
@@ +946,5 @@
>          WEBGL_depth_texture,
>          WEBGL_lose_context,
>          WEBGL_draw_buffers,
>          ANGLE_instanced_arrays,
> +        WEBGL_array_buffer_range,

We'd prefer to keep this alphabetized, if we can.

::: content/canvas/src/WebGLContextBuffers.cpp
@@ +332,5 @@
> +    // make sure the start and length are valid for the given data
> +    if (!CheckArrayBufferStartAndLength("bufferSubData", data, start, length))
> +        return;
> +
> +    CheckedInt<WebGLsizeiptr> checked_neededByteLength = CheckedInt<WebGLsizeiptr>(byteOffset) + length;

Isn't this not needed after we call CheckArrayBufferStartAndLength?

@@ +342,5 @@
>                                   checked_neededByteLength.value(), boundBuffer->ByteLength());
>  
>      MakeContextCurrent();
>  
> +    GLvoid *dataPtr = data.Data() + start;

Stars to the left.

::: content/canvas/src/WebGLContextExtensions.cpp
@@ +99,1 @@
>              // We always support this extension.

Since we're starting to pile these up, let's put them above or below the rest of the case statements.

::: content/canvas/src/WebGLContextGL.cpp
@@ +2303,5 @@
> +    if (maybeArrayViewDataType != -1) {
> +        if (maybeArrayViewDataType != requiredDataType)
> +            return ErrorInvalidOperation("readPixels: Mismatched type/pixels types");
> +    } else {
> +        if ((reinterpret_cast<uintptr_t>(validDataPtr) & (alignment - 1)) != 0)

Use a static helper func for checking alignment. We might also have something for this in MFBT.

@@ +2304,5 @@
> +        if (maybeArrayViewDataType != requiredDataType)
> +            return ErrorInvalidOperation("readPixels: Mismatched type/pixels types");
> +    } else {
> +        if ((reinterpret_cast<uintptr_t>(validDataPtr) & (alignment - 1)) != 0)
> +            return ErrorInvalidOperation("readPixels: Offset must be aligned to %d bytes for operation", alignment);

Should this really be an error? IIRC, GL supports misaligned loads. (We might have to play with the Pack values, though. We do need to check that `length` is an int-multiple of the bytesPerPixels size, though.

@@ +3399,5 @@
> +    if (IsContextLost())
> +        return;
> +
> +    if (!IsExtensionEnabled(WEBGL_array_buffer_range))
> +        return ErrorInvalidOperation("readPixels: WEBGL_array_buffer_range is not enabled");

Mostly we should just assert this, I think.

@@ +3423,5 @@
> +    MOZ_ASSERT(tex);
> +    tex->SetImageInfo(target, level, width, height, internalformat, LOCAL_GL_UNSIGNED_BYTE,
> +                      WebGLImageDataStatus::InitializedImageData);
> +
> +    ReattachTextureToAnyFramebufferToWorkAroundBugs(tex, level);

This function is a bunch of duplicated code. Can we not combine this with the function above?

@@ +3890,5 @@
>  
> +    if (pixels.IsNull()) {
> +        TexImage2D_base(target, level, internalformat, width, height, 0, border, format, type,
> +                        0, 0, -1,
> +                        WebGLTexelFormat::Auto, false);

Either both these calls should have 'return', or neither should. I think we should probably have neither do it.

::: content/canvas/src/WebGLExtensionArrayBufferRange.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "WebGLContext.h"
> +#include "WebGLExtensions.h"

Include the 'base' header first. (WebGLExtensions) Alphabetize header includes after that.

::: dom/webidl/WebGLRenderingContext.webidl
@@ +937,5 @@
>  };
> +
> +[NoInterfaceObject]
> +interface WebGLExtensionArrayBufferRange {
> +    void bufferDataWEBGL(GLenum target, ArrayBuffer data, GLuint start, GLsizei length, GLenum usage);

I think we can skip the WEBGL suffix wart. We skip it in getTranslatedShaderSource, so I'm going to use that as precedent to try to avoid these warts.

Also, based on what we have already in WebGL for bufferData and bufferSubData, it looks like we should be using GLsizeiptr for `length` and GLintptr for `start`.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> > +        if ((reinterpret_cast<uintptr_t>(validDataPtr) & (alignment - 1)) != 0)
> > +            return ErrorInvalidOperation("readPixels: Offset must be aligned to %d bytes for operation", alignment);
> 
> Should this really be an error? IIRC, GL supports misaligned loads. (We
> might have to play with the Pack values, though. We do need to check that
> `length` is an int-multiple of the bytesPerPixels size, though.

Ehhh. I think we can keep this an error for sanity.  It's readpixels already, and should be an easy fix.  We can always try to add this to the spec (readpixels args must be aligned).  I think that they effectively must be aligned already today, because you have to pass in the right view type.  But the only view type that's valid is a uint8 view, so it just kinda happens to work?

Comment 10

[u480271]

But I'm no longer working on WebGL

This work might be of interest to Jukka?

Comment 11

[Jukka Jylänki]

Heyhey, this indeed is interesting! This has come up several times in the past, didn't know there was an item about this before.

There's two advantages we could have with this:

The devtools team some time ago added a feature to the performance tools to allow profiling which sites generate JS object allocations. We've used this tool to identify and clean up places in Emscripten runtime to minimize where temporary short-lived JS object allocations occur. In some Emscripten apps, it is possible to run without generating per-frame garbage at all, which is pretty cool. Currently in most profiled applications, these WebGL function calls that require a tightly sized typed array to point to the input/output data buffer, are the biggest cause of JS object garbage we have. It would be very nice to clean this up, since in some cases, this can cause hundreds of allocated JS objects per frame, especially at times when lots of uniforms or buffers are uploaded.

The second benefit is that currently we struggle with the performance of allocating a new typed array view (to an existing typed array). Profiling shows that allocating a typed array view is relatively slow, enough slow so that if the view is very small, it is faster to preallocate a view at startup to a dummy portion of the memory, and do manual element by element copies of the data to the dummy typed array, which is then passed to WebGL. We currently do a farm of dummy typed arrays for all small sizes (1-1024 bytes), where profiling shows that only above that, it is faster to allocate a new typed array on demand. This kind of pooling was observed to give in the order of -10% of CPU time spent in per-frame rendering of the UE4 Sun Temple demo (from https://github.com/kripken/emscripten/commit/2c6e2a7687d94118de3f8401d7d023fe2cd74a30 and https://github.com/kripken/emscripten/commit/64400937fd802f14c223f0f8a9223291be26e2f0).

If there was a WebGL extension that allowed one to just pass existing typed arrays with index offset and length (requiring C alignment would be just fine, so element index instead of byte offsets would be ok), that would immediately remove both of the above inconveniences.

Comment 12

[Kelsey Gilbert [:jgilbert]]

I'll take this.
I think this is worth prototyping.

Comment 13

[Jukka Jylänki]

Sweetness! Let me know when it's useful to test out - I can whip up an Emscripten path and do some demo tests for comparison.

Comment 14

[Jukka Jylänki]

Attachment: subarray_trash.png

Profiling a game from a notable partner who is developing a Unity3D game, they are impacted by stuttering caused by Firefox GC. The allocations perf tool logger shows that most trash, ~63%, comes from the glBufferSubData() entry point, when creating a properly sized typed array view to feed to WebGL. Attached an image to show how it looks like.

Having a WebGL extension for this would be very useful!

Comment 15

[Jukka Jylänki]

Hey Jeff, I see that you pushed this towards being a feature in the WebGL2 core spec: https://github.com/KhronosGroup/WebGL/commit/988a045ade38f1acc564d1599246181d509fd309 . That looks awesome! Does FF Nightly yet have this implemented? (I suppose no since this bug is still open(?)) Looking forward to trying it out once this is available.

Comment 16

[Kelsey Gilbert [:jgilbert]]

(In reply to Jukka Jylänki from comment #15)
> Hey Jeff, I see that you pushed this towards being a feature in the WebGL2
> core spec:
> https://github.com/KhronosGroup/WebGL/commit/
> 988a045ade38f1acc564d1599246181d509fd309 . That looks awesome! Does FF
> Nightly yet have this implemented? (I suppose no since this bug is still
> open(?)) Looking forward to trying it out once this is available.

I have patches. This is in the pipeline.