And as Dr Stone would say:
I see that Buffer() constructor has a reference to engine. And there also is buffer.align.ts. I’m not sure about the aligning stride in that function, but perhaps something similar could be used when the user creates a VertexBuffer. The engine parameter can be checked for alignment requirements and the underlying ArrayBuffer created with two (or 1-3) extra bytes, while the TypedArray or DataView (whichever makes sense) is overlayed on top of it of the requested length.
If done like that, I don’t think it’d be a breaking change because the new setSubDataAlign (renamed to setSubData) would handle it seamlessly and thus more use cases would work. I don’t think that cases that worked before would now break. That is, if they break now, they would have already broken before due to incorrect alignment. I think not copying an entire buffer when alignment is needed would be a big performance win.
After all, these are functions that are calling GPUBuffer.Queue.writeBuffer(), and that requires an aligned buffer. We might have to make fixes to any functions that assume the backing ArrayBuffer is exactly the same size as the overlayed TypedArray. Do you think any user code has made that assumption?
What do you think of this approach?
I’m not sure how that should work? In my example above, the Int16Array buffer is directly created by user code (the glTF loader), not through vertex buffer creation. Even if the Buffer class created an aligned buffer and stored it internally, the source buffer would still be non-aligned, and the user could call an update function using this buffer.
Agreed. The code path doesn’t work for my idea. Then maybe the best approach is to either make this work within the new setSubData with copy and/or two writeBuffers or to make sure a warning or error issued is clear. Along with the warning/error should be a createAlignedBackingBuffer(size,oldBuffer?) that verifies, copies, or produces the larger ArrayBuffer backing a smaller TypedArray(). The error could be something like “Error in writeBuffer, buffer must be aligned. Please use createAlignedBackingBuffer() for src.”
If we decide to fix setSubData with copy, then this could be a warning “… For performance, please use…”
If this is a general problem for all VertexBuffer objects, then a warning could be issued in VertexBuffer constructor.
But, it looks like this may already be functionality in VertexBuffer?
effectiveBuffer: Nullable<DataBuffer>
Gets the effective buffer, that is the buffer that is actually sent to the GPU. It could be different from VertexBuffer.getBuffer() if a new buffer must be created under the hood because of the forceVertexBufferStrideAndOffsetMultiple4Bytes engine flag. (Though I haven’t seen a “stride” requirement in WebGPU yet).
Must all user-created buffers go through VertexBuffer before getting to the GPU? If so, I think that’s the insertion point for aligned buffers.
The only thing I don’t like is if VertexBuffer keeps the aligned buffer “under the hood” and doesn’t expose it to user, then can the user still create a data buffer that can be updated without incurring a CPU->CPU copy of data from user data to “under the hood” data? That is, how does a user write into the “under the hood” data buffer directly?
Edit: I see that createRawBuffer() in webgpuBufferManager.ts already creates an aligned buffer. It doesn’t create a larger backing buffer with a TypedArray of the requested size. It just returns dataBuffer.data as a bigger size. It’s coded such that it expects size in bytes. And elements are 1, 2, 4, or 8 bytes in size. So it will add up to 3/3 bytes/elements, 2/1 bytes/elements (for element sizes 1 or 2), and never need to add bytes for element sizes of 4 or 8 since those are already aligned.
If a user uses a function that calls new VertexBuffer, and if that goes through createBuffer() in webgpuBufferManager.ts (for WebGPU engine), then I think we’re good! The only caveat is that users should be aware that WebGPUDataBuffer.data should be used for direct writing and for setSubData instead of their original TypedArray to insure that an aligned buffer is used in setSubData. I would prefer that there’s a TypedArray overlay of the exact requested size, but I can’t yet articulate any benefits in practice (except hypothetical user code). I’d have to check any other code using setSubData to see if it would produce an error.
Edit 2: This playground shows that new Buffer result in a GPUBuffer that is aligned: the capacity is corrected to 12 bytes. However, the buffer associated with the GPUBuffer is only specified as 10 bytes. This makes writeBuffer more complicated if we want to avoid errors when writing. Note that capacity property does not exist on a WebGLBuffer. Which is a little inconsistent, but not devastating. I think this all means that the nominal code path creates a correct WebGPU buffer, but does not match it with an aligned ArrayBufferView as a data source. I’m still hopeful there is a code path that creates the data buffer in a way that is conducive to a properly working writeBuffer.
I’m running out of time this week. So I’ll revisit next week.
Here’s a playground that demonstrates the aligned backing Buffer I’ve mentioned. It’s not yet generic enough for a function yet. It also doesn’t detect what engine is used.
My first thought is to have a function that looks like
createDataWithAlignedBacking(type, elementCount);
And would be called as
createDataWithAlignedBacking(Uint16Array,5)
(Edit2: here’s the function DataTypeAligned() that does this)
The playground shows the creation of d5 (starting line 18) with an aligned backing and a call to new Buffer(). On the lower right are a variety of size-related properties. I haven’t fully verified that using this kind of buffer reduces CPU-CPU copying during writeBuffer, but my hope is that it will. I also think this would be a good candidate for including in the path of Buffer creation with engine-specific detection (only create aligned backing when the engine requires it). I don’t think this fully resolves the issue if a user creates an independent non-aligned TypedArray then uses that array with setSubData (which calls writeBuffer).
The end result I think will be recommended easy step(s) to create an array with the correct backing alignment and works fastest with setSubData and GPUBuffer.
Sorry, I’m a bit slow to reply, but I’m a bit busy with urgent work at the moment, so I’ll be able to get back to you early next week!
Here’s a proposed function outside of the framework (Buffer codepath) that aligns a TypeArray when requested (untested). The third argument (optional) is the engine, an alignment number, or value true (for auto detecting engine). Unspecified alignment returns a standard new TypeArray. It is totally suitable for use in the Buffer codepath, including inside engine-specific code. It’s also usable by the user when creating a TypedArray for subsequent use with Buffer, VertexBuffer, and VertexData. It’s unclear whether retrieved VertexData is suitable for use with setSubData, but that is a modification to the framework that I would propose.
I’ll have a chance to test it in the coming week. Just placing it here for discussion.
This function creates a TypedArray with optional byte alignment of the backing ArrayBuffer size. The returned TypeArray has the requested element count, regardless of alignment requested.
Designed for creating an aligned buffer for use with VertexBuffer, the arguments allow flexible query of engine feature specifying alignment, but returning as quickly as possible in the “most likely uses” below.
Most likely uses will be one of
- TypeArrayAligned(type,count) // no alignment
- TypeArrayAligned(type,count,true) // uses BABYLON.EngineStore.LastCreatedEngine
- TypeArrayAligned(type,count,engine)
Or for other purposes not using engine, call with a number
- TypeArrayAligned(type,count,2)
- TypeArrayAligned(type,count,4)
Recommended examples (in order of lowest speed, minor differences)
- TypeArrayAligned(type,count,1)
- TypeArrayAligned(type,count) // or 0, null, undefined
- TypeArrayAligned(type,count,4) // or any number
- TypeArrayAligned(type,count,engine._features?.forceVertexBufferStrideAndOffsetMultiple4Bytes?4:1) // fastest embedded when you have engine var
- TypeArrayAligned(type,count,engine) // queries ._feature, same as above
- TypeArrayAligned(type,count,true) // easiest way to auto engine check, grabs engine from BABYLON.EngineStore.LastCreatedEngine
- TypeArrayAligned(type,count,false) // same as no alignment
(Untested as of posting)
function TypeArrayAligned(typeArray, elementCount, engineOrAlignment=1){
// tuned to be fastest with 1 but handling other values gracefully
if (engineOrAlignment==1 || engineOrAlignment?0==0)
return typeArray(elementCount)
// here, we've already done two/three comparisons
// and eliminated 0, 1, null, and undefined
if (engineOrAlignment>1) {
align = engineOrAlignment
} else { // definitely an engine or boolean at this point
If (engineOrAlignment == true)
engineOrAlignment = BABYLON.EngineStore.LastCreatedEngine
align = engineOrAlignment?._features?.forceVertexBufferStrideAndOffsetMultiple4Bytes?4:0
if (align <= 1) return typeArray(elementCount)
}
// if we've reached this point, align is a number > 1
var size = elementCount*typeArray.BYTES_PER_ELEMENT+align-1
size -= size%align
const backing = new ArrayBuffer(size)
return typeArray(backing,0,elementCount)
}
No, vertex buffers are one kind of buffer, but you can have other kinds of buffers (index buffers, storage buffers, uniform buffers, etc). Vertex buffers have unique constraints, which are handled by the forceVertexBufferStrideAndOffsetMultiple4Bytes engine flag.
It’s in the 10.3.7.1 section:
You can use methods that do this for you (such as VertexBuffer.updateDirectly or StorageBuffer.update). You can also use certain methods directly on the engine (updateDynamicVertexBuffer, updateDynamicIndexBuffer, updateStorageBuffer), which take a DataBuffer (a small wrapper around a WebGL or WebGPU buffer) as a parameter. But you don’t have direct access to bufferSubData (WebGL) or writeBuffer (WebGPU), because we have to hide these engine-specific methods from the user.
I don’t really understand the TypeArrayAligned method and its usefulness… Testing forceVertexBufferStrideAndOffsetMultiple4Bytes is a mistake, because this engine feature is specific to vertex buffers, it doesn’t apply to other types of buffers. In addition, forceVertexBufferStrideAndOffsetMultiple4Bytes=true is more involved than simply aligning the buffer size (see the implementation in VertexBuffer._alignBuffer).
I don’t think we should over-engineer the WebGPUBufferManager.setSubData method, as long as the path most often used (sizes and offsets aligned to multiples of 4 bytes) is the fastest. I agree that giving feedback to the user when this isn’t the case would be nice, but logging an error could fill up the console quickly and slow down the application… I wish we had a debugging mode in Javascript, where some of the code could be removed in release mode!
Have you been able to take a look at the current draft PR?
TypedArrayAlign would be used by the user to create a CPU buffer that contains data that later is tied to a GPUBuffer. Such a CPU buffer could then be updated by the user (using the returned TypeArray) and subsequently used with the new setSubData without error, because it uses the backing array with the extra (empty) bytes. This only comes into play when writing the last elements in the array.
If possible, TypedArrayAlign would be inserted in the WebGPU engine code that creates and returns a TypedArray for the user to update.
Because the user seems more likely to independently create a buffer, then associate it with the GPUBuffer, then use that independent buffer for updates, there is not a nominal code path in which the user obtains a suitable buffer for those updates. TypedArrayAlign gives the user that nominal code path. It should be used whenever the size of the element type is not an integer multiple of the required alignment and the element’s number * bytes is not a muultiple of the alignment. The code within TypedArrayAlign was intended to be an automatic detection of those conditions.
Agreed that testing for forceVertexBufferStrideAndOffsetMultiple4Bytes is incorrect.
I looked at the updated PR and it looks good to me! Let me know if you’d like me to go through it again for a more confident concurrence.
Yes, I agree that adding a createAlignedTypeArray method could be useful. My only doubt is where to add it. Engine (AbstractEngine) would be the most obvious class, but I know we want to keep these low-level classes as light as possible, so I’m not sure, because it’s just an helper function… cc @sebavan for opinion.
Is it possible to keep that function external and not associated with an engine? Or do we want it to be engine specific? (I did not read the thread sorry)
Yes, the function is engine specific, as for WebGL we would simply create the typed array and would let ANGLE do its magic. We can pass the engine to the function, but the question is: where to put this function?
I see, so you are right it should be in AbstractEngine but abstract so no impact on size and then implemented by engine and webgpuengine respectively
Yes, it appears to do exactly as needed for use with a GPUBuffer.
PR has been updated, if you want to review it: