这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@kainino0x
Copy link
Contributor

@kainino0x kainino0x commented Mar 28, 2019

Taking ArrayBuffer as input is problematic: if an application wants to provide a sub-range of an existing ArrayBuffer, it must make a copy of all of the data into a new ArrayBuffer.

  • setSubData: Uses ArrayBuffer BufferSource + srcOffset + size to avoid the garbage overhead of creating an ArrayBufferView.
  • GPUShaderModuleDescriptor: creating pipelines is already very expensive, so this just takes an ArrayBufferView a BufferSource a Uint32Array.

@kainino0x
Copy link
Contributor Author

FYI, WebGL does this with (ArrayBufferView, srcOffset, length). This is also fine, it just makes it unclear whether srcOffset and length are in units of bytes or multiples of BYTES_PER_ELEMENT.

@kainino0x
Copy link
Contributor Author

I remembered BufferSource was a thing, and updated accordingly.

@kainino0x
Copy link
Contributor Author

Thinking about it more it doesn't really make sense to pass ArrayBuffers into these entry points, since ArrayBuffers don't semantically expose access to their contents (a view is required). Updated.

@kainino0x kainino0x requested review from Kangz and kdashg and removed request for kdashg April 2, 2019 16:06
@jsheard
Copy link

jsheard commented Apr 2, 2019

BufferSource would be easier for the WebAssembly case, since the WebAssembly.Memory object contains a plain ArrayBuffer with no views. Wrapping the wasm memory in an ArrayBufferView is annoying because the view needs to be cached to avoid garbage, but the view becomes detached when wasm memory grows, so there needs to be a check if the cache is still valid every time it's used.

@magcius
Copy link

magcius commented Apr 2, 2019

ArrayBufferViews are not great for packed buffers, where you will be using DataView to fill in the contents.

@kainino0x
Copy link
Contributor Author

In practice, IIRC, Emscripten always keeps around one of each type of view into it's entire heap, and does reset them when the heap grows. It uses these views every time it needs to access the heap from JS, because it's the only way.

Good point about DataView (which is especially useful for uniform buffers). I thought of it before but forgot when I made this change. Still, semantically I think it makes more sense to make a view into the buffer behind the DataView, for the reasons above. Or allow DataView to be passed in here.

OTOH if we get rid of setSubData then it's moot; DataView isn't particularly useful for SPIR-V code.

@magcius
Copy link

magcius commented Apr 2, 2019

To me, I see the ArrayBuffer as an opaque blob of data (void*), and the view as a pointer into that data (float*, uint32_t*). This analogy is stretched a bit since views can view partial ArrayBuffer contents through the offset/length parameters, yet there's no way to slice an ArrayBuffer without copying (a very unfortunate omission, in my opinion). Vertex buffers are a case where you might have 3 F32 fields for position, six SNORM16 fields for normal/tangent/bitangent pairs, two UN16 fields for texture coordinate, four U8 fields for color, etc. Similarly with texture data, or bytecode, etc. There's no consistent view into that data from an ArrayBufferView. Will SPIR-V module creation demand a Uint32Array, since everything is packed in 32-bit words? And before you laugh, WebGL requires this for texture uploads today -- you have to pass a Int16Array for a 16-bit signed integer texture, a Float32Array for floating-point textures, etc.

You can always access the underlying ArrayBuffer from a view for free, but creating the view to wrap a view is garbage. So I would much prefer ArrayBuffer.

@kainino0x
Copy link
Contributor Author

Allowing only ArrayBuffer would be kind of unwieldy; if you do have an ArrayBufferView, it requires unpacking it into (ArrayBuffer, offset, size) manually in JS.

Also, if createShaderModule only takes ArrayBuffer and not ArrayBufferView, we have to add the offset and size arguments to GPUShaderModuleDescriptor, and, again, users have to unpack them manually. As I said above, since the cost of createShaderModule is already so high, I think I'd prefer to stick with the web platform's existing primitives and take only ArrayBufferView. I actually would be fine with requiring this to be Uint32Array, as it is semantically correct.

And if setSubData is gone (it kind of seems like it's going that way), this is the only entry point left for now.

@magcius
Copy link

magcius commented Apr 3, 2019

Sounds good then; thanks. Didn't realize that setSubData was heavily on the chopping block.

@kainino0x
Copy link
Contributor Author

Thanks for all your input. I'll be sure to re-raise this if we don't remove it.

@Kangz
Copy link
Contributor

Kangz commented May 22, 2019

@kainino0x can you rebase this? Also shouldn't the variant for SPIR-V be Uint32Array?

@kainino0x
Copy link
Contributor Author

Rebased.
I think you're right, we should use Uint32Array here - it semantically makes sense, as I said above.

But that said, I recall WebGL was adding entry points for (ArrayBuffer, offset) to make things work better when resizing WASM heaps. Should we take that approach? (cc @kenrussell)

@kainino0x
Copy link
Contributor Author

Chatted with Ken about this and we decided it's best to keep things simple for now and use Uint32Array+offset. In the future we might want to change it to BufferSource+offset, which would be a generalization.

@kainino0x
Copy link
Contributor Author

Er, per my previous comments, the cost of createShaderModule is so high compared with the cost of garbage collecting one Uint32Array wrapper, that it's nicer to just pass Uint32Array here instead of Uint32Array+offset+length.

Ready for review/landing. Note this PR now only affects ShaderModuleDescriptor (as setSubData is gone), and only the SPIR-V version of it.

@kdashg
Copy link
Contributor

kdashg commented May 22, 2019 via email

Copy link
Contributor

@kdashg kdashg left a comment

Choose a reason for hiding this comment

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

Sounds good to me, and to the rest of the WG.

@kdashg kdashg merged commit c675051 into gpuweb:master Jun 3, 2019
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
)

* Adding a TODO for copy between linear data and texture tests

That should be covered by something like copyBetweenLinearDataAndTexture_bufferRelated.spec.ts.

* Splitting TODO to TODOs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants