Guaranteed-efficient mapping #511
Conversation
Remove createBufferMapped as now redundant.
This guarantees that mappings are direct and efficient by programatically forbidding mapping if the application does not handle fences in a way that guarantees mutually-exclusive usage/mapping.
kvark
left a comment
There was a problem hiding this comment.
I like the general direction/simplicity of where this is going.
While currently we have both mapping and fences overlapping in functionality, this PR makes mapping effectively piggy-back on the fences, and in general simplifies the API.
What concerns me is the "guarantee" here, which appears optimistic.
| - "<dfn dfn for="buffer state">ready</dfn>" where the {{GPUBuffer}} is available for {{GPUBuffer/map()}} or {{GPUQueue.submit()}}. | ||
| - "<dfn dfn for="buffer state">mapped</dfn>" where the {{GPUBuffer}} is available for CPU operations. | ||
| - "<dfn dfn for="buffer state">unmapped</dfn>" where the {{GPUBuffer}} is available for GPU operations. | ||
| - "<dfn dfn for="buffer state">executing</dfn>" where the {{GPUBuffer}} is in-use for GPU operations. |
There was a problem hiding this comment.
Is the transition from "executing" to "ready" defined by something user does (e.g. waiting on a fence), or happening behind the users back?
|
|
||
| ## {{GPUBuffer/map()}} ## | ||
|
|
||
| If the devices is lost, returns null. |
| If {{GPUBuffer/map()}} does not include {{GPUMap/WRITE}}, writes to the ArrayBuffer are ignored. | ||
|
|
||
| With {{GPUQueue/createFence()}} and awaiting {{GPUFence/onCompletion()}}, it's possible | ||
| to ensure that {{GPUBuffer/map()}} provides an efficient mapping to memory with a minimal |
There was a problem hiding this comment.
this needs to be clarified: "possible" to whom?
It seems like it's possible for both the implementation to track buffer usage in submissions and fences, and the user to know which fence to wait on before trying to re-map a buffer, and only if both of these are accomplished the mapping becomes "guaranteed-efficient".
| of {{GPUFence/[[pendingfenceIDBySignalValue]]}} at key signalValue. | ||
| * Remove {{GPUFence/[[pendingfenceIDBySignalValue]]}} at key signalValue. | ||
|
|
||
| If {{GPUBuffer/map()}} does not include {{GPUMap/READ}}, reads from the ArrayBuffer are zeros. |
There was a problem hiding this comment.
It's not mentioned anywhere but I assume that if the flags include GPUMap_READ then the content of the ArrayBuffer is the content of the [[buffer]] at that point.
There is no signal from the application whether it wants to read the content of the buffer or wait for later. And to guarantee the semantics outlined in this PR, the browser would have to eagerly send the content of each ready MAP_READ buffer, after each fence is passed so that when the user sees the fence passed, the content of the buffer is already on the content process side.
There was a problem hiding this comment.
I agree with this, even though in many cases reading the buffer when it's "ready" is totally fine:
- either the user keeps using the buffer on GPU and it never actually reaches "ready"
- or the user really needs to read from it, hence the copy is needed
I wonder if this particular issue can be addressed by making the "request to read" explicit in the API as a queue operation, i.e. the user says queue.makeBufferReadable(buffer, range), and if they put a fence after that they have a guarantee that the buffer can be read.
Have to acknowledge that this looks very similar to a hypothetical queue.readBufferData that we could to #509 in line with the API it exposes.
There was a problem hiding this comment.
Yeah, basically we need some form of explicit asynchrony and any solution here will be
conceptually similar to mapReadAsync or queue.readBufferData.
There was a problem hiding this comment.
queue.readBufferData could work one of two ways:
- It returns a
Promise<ArrayBuffer>, or - It doesn't return anything, and after the app waits on a fence, they can synchronously get the results.
If we're going to do https://github.com/gpuweb/gpuweb/pull/509/files queue.writeBuffer() and queue.writeTexture(), it makes sense to add queue.readBufferData() for symmetry.
There was a problem hiding this comment.
Just a wild idea if we can detach an ArrayBuffer passed to readBufferData and then "re-attach" it whenever the promise resolves and it has the data we've just written. That would allow the user to avoid creating new objects.
| If {{GPUBuffer/map()}} includes {{GPUMap/WRITE}} but the buffer is not marked for | ||
| {{GPUBufferUsage/MAP_WRITE}}, generate an error and return null. | ||
|
|
||
| When buffers are used in a {{GPUCommandBuffer}} sent successfully to |
There was a problem hiding this comment.
This requires client side state tracking for GPUBindGroup, GPURenderBundle, GPURenderBundleEncoder, GPURenderPassEncoder, GPUComputePassEncoder, GPUCommandEncoder and GPUCommandBuffer which would be very unfortunate (let's say -3).
| * Remove {{GPUFence/[[pendingfenceIDBySignalValue]]}} at key signalValue. | ||
|
|
||
| If {{GPUBuffer/map()}} does not include {{GPUMap/READ}}, reads from the ArrayBuffer are zeros. | ||
| If {{GPUBuffer/map()}} does not include {{GPUMap/WRITE}}, writes to the ArrayBuffer are ignored. |
There was a problem hiding this comment.
This forces an extra copy for GPUMap_READ otherwise an application would be able to map for reading and start writing bytes in memory.
|
|
||
| On {{GPUQueue/createFence()}} and {{GPUQueue/signal()}}: | ||
| * In {{GPUFence/[[pendingfenceIDBySignalValue]]}}, assign key signalValue to value | ||
| {{GPUQueue/[[nextFenceID]]}}. |
There was a problem hiding this comment.
Perhaps would be worth adding some wording somewhere about how you expect this to work with multi-queue. Are you supposed to wait on a fence on all queues? That seems clearly too overkill. On no queues? That’s incompatible with a GPU-Process architecture. All the queues where there are in-flight commands which reference the resource? What if there are 0 in flight commands which use the resource, then this isn’t compatible with a GPU process.
|
Closing since we agreed on a proposal in the spirit of #605 |
- Some TODOs have been completed, remove them from TODO list. - Start a query without draw call is allowed, move them to operation tests. - Add pipelineStatistics checking in pipeline_statistics.spec.ts
The bikeshed syntax here is mostly wrong, so cleanup is needed, but I wanted to get the idea out there for discussion.
Preview | Diff