Expose the buffer state as an attribute #3014

kainino0x · 2022-06-06T22:29:44Z

spec/index.bs

github-actions · 2022-06-29T23:00:31Z

Previews, as seen when this build job started (b1f0692):
WebGPU _{webgpu.idl | Explainer | Correspondence Reference}
WGSL _{grammar.js | wgsl.lalr.txt}

kainino0x · 2022-06-29T23:02:20Z

Sketched some possibilities on how to expose this
https://docs.google.com/spreadsheets/d/1p5xAB0sh-jAj493RXMeq2Kvbq9LlofEnaGVF86fkcO8/edit?usp=sharing

Kangz · 2022-07-01T09:33:01Z

I thought I remember a discussion that "unmapped" is unclear but couldn't find it again. I made proposal 4 which is proposal 2 but uses the name "unmapped". I think "mappable" would be misleading because some buffers are in the unmapped state but aren't mappable so that enum value would be confusing. IMHO developers will understand that unmapped means "usable on the GPU".

The other thing is that I think we should make the distinction between pending and unmapped because otherwise there isn't a way to know if the buffer can be used in a submit.

kainino0x · 2022-07-25T22:00:02Z

[Editors]
Myles points out there could be a difference in what the application does in response to "not mapped" vs "pending". For example:

"not mapped" -> try to map it
"pending" -> wait for mapping

Most likely there isn't any value in trying to hide this detail from the user, so we should probably expose it. That said, I'm in the middle of trying to work through the detangling of content-private vs content-shared vs device-timeline state and that might provide an answer to this issue.

kainino0x · 2022-07-26T23:01:32Z

Another thought: the "locked" state probably isn't necessary. In particular, we wouldn't be guaranteeing that, if a buffer is not "locked", the map will actually succeed. It can still fail due to mapAsync validation (e.g. already mapped, destroyed, doesn't contain usage, etc.)

With multithreading, we also don't really need to expose a "locked" state when mapped on another thread - that'll be covered by the existing validation check in mapAsync that checks the buffer isn't mapped already.

So revising this proposal to have three states: "unmapped", "pending", and "mapped".

kainino0x · 2022-07-27T00:27:10Z

Note with this proposal, "destroyed" can be a purely service side state as I have been hoping. In fact, the entire "buffer state" can move to the service side, which I've now gone ahead and done.

Note in the multithreaded future, "unmapped" and "pending" mean just on this thread/this GPUBuffer object. It might also be in any state on another thread.

Kangz · 2022-07-28T15:28:03Z

So revising this proposal to have three states: "unmapped", "pending", and "mapped".

Revised proposal and skimming the diff, things look good, thanks!

toji

Looks good, but don't we need to transition the mapState back to "unmapped" in the .unmap() algorithm? That's missing here.

kainino0x · 2022-08-03T17:23:02Z

Looks good, but don't we need to transition the mapState back to "unmapped" in the .unmap() algorithm? That's missing here.

Oops, forgot this. That algorithm is kind of nonsensical right now so we also need to fix that soon. Maybe with #3264.

github-actions · 2022-08-03T17:24:21Z

Previews, as seen when this build job started (4219789):
WebGPU _{webgpu.idl | Explainer | Correspondence Reference}
WGSL _{grammar.js | wgsl.lalr.txt}

spec/index.bs

github-actions · 2022-08-03T23:48:35Z

Previews, as seen when this build job started (62b5c10):
WebGPU _{webgpu.idl | Explainer | Correspondence Reference}
WGSL _{grammar.js | wgsl.lalr.txt}

kdashg · 2022-08-04T01:06:14Z

GPU Web meeting 2022-08-02/03 APAC-timed

KN: This has gone through a bunch of revisions. I’ve been working through state on client vs server for buffer mapping. What I came to is that client always needs to know whether it’s pending, can’t throw that away, so needs at least three states on client side. Observation from editors meeting from MM, is that it could be useful to know whether it’s mapped vs mappable. Also, it’s probably not worth hiding this from authors, and we should just let them check.
KN: So unmapped/pending/mapped should work well, shouldn’t be onerous, and all three should be useful for devs, so that’s my proposal.
KN: For context, in Try-catching getMappedRange is useful but shouldn't be #3013, our resolution was that we should probably just expose this, so this proposal is that we just expose this.
MM: Discussing this internally. We feel that it this is tracked on client, it’s useful to expose the enum, rather than a more arcane way to get this info.
MM: Another option here (not advocating particularly) is that we don’t have any state on the client.
KN: Sort of jives with some stuff I’m considering but I don’t think it’s possible.
KG: I think we determined in Firefox it was possible but onerous. Cases where if you map a buffer and unmap it you’ll get the promise back with the shmem. And we’d have to then send it back and say “just kidding”.
JB: We decided not to do this. Would have let us use some prebuilt shmem ownership tracking but decided it would be too baroque. Now using “unsafe shmem”.
KN: Thought about this a bunch, I’m pretty sure that it’s not really possible to store no state on the client. The client needs to know certain things.
MM: I think it’s possible to be somewhat simpler via tracking generation ids for these exchanges.
KN: different client side state, but still have some.
JB: Corentin said one of the reasons for behavior being specified the way it is is wanting to allow for post-v1 multiple mapAsync regions on the same buffer. It would be possible to have multiple regions mapped. In this proposal Expose the buffer state as an attribute #3014, the state reported would be misleading.
KN: I think there is room for support for sub-range mapping, but it gets a little strange, e.g. “pending” with some-pending-some-unmapped sub-ranges. But we’d also probably need a new e.g. Unmap() function too, since right now that’s only for the whole buffer.
MM: Could have multi-map be a new thing that just exposes a “multi-mapped” state.
JB: Then if they called multi-map they could get that state. If they don’t then they can’t get the new state.
KN: MM’s suggestion makes sense. Think we can always make it work somehow.
KN: “mappable buffer views” were another proposed API for this and wouldn’t have as much of an issue.
…
KN: Note multi-mapping would be in a world where you can also use parts of the buffer that aren’t mapped.
KG: … Consensus was basically we had better things to do for V1.
(Let’s come back to this next week)

kainino0x · 2022-08-10T19:48:35Z

Meeting resolution: Agreed but adding to tacit resolution for @litherum to give input at the next meeting (or earlier)

This change tries not to have behavior effects compared to what we have specced now, but since what we have now doesn't quite make sense, it picks just one of several possible interpretations of what we had written. #3271 is open about which one we actually pick. **I'm calling this editorial despite picking an interpretation it leaves an open issue to figure out which one to use.** This is now independent of exposing the buffer state as an attribute (#3014). This change defines a number of content-side states from which that attribute would be defined. The notable changes are: - Server side now has just 2 states: available, unavailable. - Client side has two state variables: - `[[mapping]]` which is either null, or all of the stuff for an actively-mapped buffer (not pending). - `[[map_requests]]` which tracks all map requests that haven't yet gotten a response from the device. - At least in the mapping methods, client states are only accessed on the client, and server states are only accessed on the server. - `unmap()` is rewritten to deinterlace the client and server side steps. - **`unmap()` doesn't reject promises. Instead it marks requests as cancelled so they get rejected when the map request completes.**

github-actions · 2022-08-17T01:54:18Z

Previews, as seen when this build job started (510d7a8):
WebGPU _{webgpu.idl | Explainer | Correspondence Reference}
WGSL _{grammar.js | wgsl.lalr.txt}

kainino0x · 2022-08-17T22:46:16Z

was only waiting for approval from Apple, and got explicit approval so landing now.

It is added to the WebGPU spec at gpuweb/gpuweb#3014 bug: dawn:1521 Change-Id: If6ed910f8ec0ca0df678c38881188cb25630c437 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4145218 Reviewed-by: Austin Eng <enga@chromium.org> Commit-Queue: Takahiro <hogehoge@gachapin.jp> Reviewed-by: Corentin Wallez <cwallez@chromium.org> Cr-Commit-Position: refs/heads/main@{#1090606}

kainino0x added the proposal label Jun 6, 2022

kainino0x marked this pull request as ready for review June 6, 2022 22:29

This comment was marked as outdated.

Sign in to view

This was referenced Jun 6, 2022

Try-catching getMappedRange is useful but shouldn't be #3013

Closed

Return null from getMappedRange if mapping is pending #3010

Closed

kainino0x added this to the V1.0 milestone Jun 6, 2022

Kangz reviewed Jun 7, 2022

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

kainino0x added the for webgpu editors meeting label Jun 15, 2022

kainino0x added the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Jun 29, 2022

kainino0x added the tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue label Jul 25, 2022

kainino0x force-pushed the buffer-state branch 2 times, most recently from f514923 to 3ebbf95 Compare July 27, 2022 00:26

kainino0x force-pushed the buffer-state branch from 78bb911 to c853cd6 Compare July 27, 2022 23:26

This was referenced Jul 28, 2022

Object labels require client-shared state #3263

Closed

Simplify device-timeline map states #3264

Closed

toji requested changes Aug 2, 2022

View reviewed changes

kainino0x self-assigned this Aug 3, 2022

kainino0x requested a review from toji August 3, 2022 17:23

toji approved these changes Aug 3, 2022

View reviewed changes

kainino0x commented Aug 3, 2022

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

kainino0x mentioned this pull request Aug 3, 2022

GPUBuffers shouldn't have to accumulate a list of mapAsync promises to reject #3271

Closed

kainino0x mentioned this pull request Aug 10, 2022

Separate content/device internal states of GPUBuffer #3300

Merged

kainino0x added tacit resolution queue Editors have agreed and intend to land if no feedback is given and removed for webgpu editors meeting tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue labels Aug 10, 2022

This comment was marked as duplicate.

Sign in to view

kainino0x force-pushed the buffer-state branch from 62b5c10 to b4b0589 Compare August 17, 2022 01:49

Expose the buffer state as an attribute

510d7a8

kainino0x force-pushed the buffer-state branch from b4b0589 to 510d7a8 Compare August 17, 2022 01:49

kdashg approved these changes Aug 17, 2022

View reviewed changes

kainino0x merged commit 0af1a24 into gpuweb:main Aug 17, 2022

kainino0x deleted the buffer-state branch August 17, 2022 22:45

kainino0x removed the tacit resolution queue Editors have agreed and intend to land if no feedback is given label Aug 17, 2022

juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 18, 2022

IDL: Add support for wgpu_buffer_map_state(). gpuweb/gpuweb#3014

fd13508

lokokung removed the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Oct 14, 2022

greggman mentioned this pull request Jan 4, 2023

op: reflections:buffer,* gpuweb/cts#2112

Open

This was referenced Jan 6, 2023

op: Add Buffer.mapState test gpuweb/cts#2124

Open

Add Buffer.mapState test gpuweb/cts#2130

Merged

Expose the buffer state as an attribute #3014

Expose the buffer state as an attribute #3014

Uh oh!

Conversation

kainino0x commented Jun 6, 2022 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

This comment was marked as outdated.

github-actions bot commented Jun 29, 2022

Uh oh!

kainino0x commented Jun 29, 2022

Uh oh!

Kangz commented Jul 1, 2022

Uh oh!

kainino0x commented Jul 25, 2022

Uh oh!

kainino0x commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kainino0x commented Jul 27, 2022

Uh oh!

Kangz commented Jul 28, 2022

Uh oh!

toji left a comment

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Aug 3, 2022

Uh oh!

github-actions bot commented Aug 3, 2022

Uh oh!

Uh oh!

github-actions bot commented Aug 3, 2022

Uh oh!

kdashg commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kainino0x commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as duplicate.

github-actions bot commented Aug 17, 2022

Uh oh!

kainino0x commented Aug 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kainino0x commented Jun 6, 2022 •

edited by pr-preview bot

Loading

kainino0x commented Jul 26, 2022 •

edited

Loading

kdashg commented Aug 4, 2022 •

edited

Loading

kainino0x commented Aug 10, 2022 •

edited

Loading