Skip to content
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.

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

Add explainer document for BufferOperations #147

Merged
merged 3 commits into from Feb 7, 2019
Merged

Conversation

Kangz
Copy link
Contributor

@Kangz Kangz commented Dec 11, 2018

This follows discussions on #138

PTAL all! @kainino0x @kvark @jdashg @litherum @RafaelCintron

@kainino0x
Copy link
Contributor

Rendered

design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
Assuming there is a single queue, there are two types of commands in WebGPU:

- "Buffered commands": commands stored inside `WebGPUCommandBuffer` like `setVertexBuffers` or `draw`.
- "Unbuffered commands": Other commands like creating objects, `WebGPUBuffer.setSubData` and `WebGPUQueue.submit`.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's great to finally have terminology for this distinction - it's been getting confusing.
Multiqueue is going to change a lot though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep but I expect to have a similar concept of ordering (just as a general DAG instead of a line) for multi-queue.

design/BufferOperations.md Show resolved Hide resolved
design/BufferOperations.md Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
- **Available**: the content of the buffer is visible.
In that state `pointer` is an `ArrayBuffer` representing the content of the buffer, `isPending` is `false` and `promise` is resolved.
- **Invalidated**: the content of the buffer is no longer visible.
In that state `pointer` is `null`, `isPending` is `false` and `promise` is rejected.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider calling this state "invalid" instead of "invalidated". I've always associated the term 'invalidated' with a display region that needs to be repainted or that something was invalid in the past. 'Invalid', on the other hand, implies something about the current state of the object. Since invalid WebGPUMappedMemory objects can never go from the invalid state to any other state, this also lends a degree of finalness to the name.

design/BufferOperations.md Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
@Kangz
Copy link
Contributor Author

Kangz commented Dec 12, 2018

PTAL again at the "Buffer mapping" section that changed heavily to use Promise<ArrayBuffer> instead of WebGPUMappedMemory after a discussion I had with @beaufortfrancois. To be clear, there is still value in being able to poll the promise, but this can be shimmed.

The "examples" section is new too.

@kainino0x
Copy link
Contributor

kainino0x commented Dec 12, 2018

On the topic of promises, I'm pretty sure that having only a regular promise (even without polling api) should be technically okay for the main thread as long as it doesn't impact latency. We already have a restriction that the mapping "can only change state on task boundary", so I don't think it would impact latency, and the following code is equivalent to what we would do internally to the browser anyway:

function pollableMapWriteAsync(buffer, offset, size) {  // [edited]
  const mapping = { pointer: null };
  buffer.mapWriteAsync(offset, size).then(ab => { mapping.pointer = ab; });
  // edit: probably add .catch(e => { mapping.error = e; }); or something
  return mapping;
}

(pending = mapping.pointer == null; available = mapping.pointer.length > 0; invalid = mapping.pointer.length == 0)

I think it only becomes a problem on Workers if we lift the "can only change state on task boundary" restriction (i.e. allow spin polling). Although with the nested event loop thing I was prototyping, it's not technically necessary because promises can resolve "synchronously" (latency could be higher though, I'm not sure).

@Kangz
Copy link
Contributor Author

Kangz commented Dec 13, 2018

@kainino0x I think you meant pollableMapWriteAsync (instead of Sync). +1 on your analysis otherwise.

@grorg
Copy link
Contributor

grorg commented Jan 14, 2019

This was discussed at the 14 Jan 2019 Teleconference

@Kangz Kangz force-pushed the buffer-ops branch 3 times, most recently from 0b54ea1 to 820c369 Compare January 22, 2019 00:43
@Kangz
Copy link
Contributor Author

Kangz commented Jan 25, 2019

PTAL again @RafaelCintron @litherum @kvark, squashed and rebased after the WebGPU -> GPU change.

design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Show resolved Hide resolved
```webidl
partial interface GPUDevice {
(GPUBuffer, ArrayBuffer) createBufferMapped(GPUBufferDescriptor descriptor);
Promise<(GPUBuffer, ArrayBuffer)> createBufferMappedAsync(GPUBufferDescriptor descriptor);
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we promising the buffer here? We could allow the user to start recording some operations on the buffer at this point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bit less elegant but could be useful. Do you have a usecase in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose there aren't that many use-cases that wouldn't be happy with either sync or async version of this call, so I'm fine with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to link these discussions, this is the same as the question with tryCreate* (#197) and createReady* (#184). Should you be able to start using something that's not ready, in prep to (maybe) use it later?

design/BufferOperations.md Outdated Show resolved Hide resolved
design/BufferOperations.md Outdated Show resolved Hide resolved
@Kangz
Copy link
Contributor Author

Kangz commented Feb 1, 2019

We agreed on the general approach at the F2F and only minor comments on this PR. If there are no big concerns, I think we should merge this and iterate with issues / PR.

@kvark
Copy link
Contributor

kvark commented Feb 1, 2019

No big concerns from me, other than the ones I explained at F2F 🤐

@Kangz
Copy link
Contributor Author

Kangz commented Feb 7, 2019

There hasn't been any comment on this for a week, I'll land it and we can iterate more in follow-up pull requests.

@Kangz Kangz merged commit 6224d4e into gpuweb:master Feb 7, 2019
@Kangz Kangz deleted the buffer-ops branch February 7, 2019 14:03
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
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.

None yet

6 participants