-
Notifications
You must be signed in to change notification settings - Fork 159
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
Introduce a "transferable" algorithm for BufferSource #1419
Conversation
The new "detachable" algorithm allows an invoker to determine if a BufferSource is detachable without errors before invoking transfer. This is particularly helpful if several buffers are processed in a batch, as the validity of the operation can be determined up front, rather than during processing which could leave the buffers in a mix of states. See discussion in [1]. A local copy of the algorithm was landed as [2] but this seems like a useful addition to WebIDL. 1: webmachinelearning/webnn#351 2: https://webmachinelearning.github.io/webnn/#buffersource-detachable
@domenic - could you please take a look? Low priority. Also, feedback on whether the checklist (re: implementation commitments, etc) is needed in this case would helpful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, happy to have this here. Just one question.
I don't think the PR template is necessary for this kind of definition.
1. If |jsArrayBuffer| has a \[[ViewedArrayBuffer]] internal slot, then set |jsArrayBuffer| to | ||
|jsArrayBuffer|.\[[ViewedArrayBuffer]]. | ||
1. If [$IsSharedArrayBuffer$](|jsArrayBuffer|) is true, then return false. | ||
1. If [$IsDetachedBuffer$](|jsArrayBuffer|) is true, then return false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this step is a bit surprising. Is it helpful for your use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully I'm not misunderstanding your question. The use case is over at https://webmachinelearning.github.io/webnn/#mlnamedarraybufferviews-transfer
Shorter version for explanatory purposes:
- For each buffer of buffers:
- If buffer is not detachable, then throw a TypeError.
- For each buffer of buffers:
- Let xfer be the result of transferring buffer's underlying buffer.
The "IsDetachedBuffer" check is to ensure that transferring wouldn't assert ("Assert: IsDetachedBuffer(jsArrayBuffer) is false").
This could alternately be done by the caller, e.g.:
- For each buffer of buffers:
- If buffer has a [[ViewedArrayBuffer]] internal slot, then set buffer to buffer.[[ViewedArrayBuffer]].
- If IsDetachedBuffer(buffer) is true, then throw a TypeError.
- If buffer is not detachable, then throw a TypeError.
- For each buffer of buffers:
- Let xfer be the result of transferring buffer's underlying buffer.
... but it seems we'd want to keep this logic together in WebIDL. Would it be clearer if we named the new algorithm transferable instead?
(But maybe I'm answering the wrong question?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, sorry for being brief.
What I mean is that even if IsDetachedBuffer() returns true, DetachArrayBuffer() will still work. (It will just be a no-op.) So it's a bit surprising to include this step, since if it was missing, the algorithm would still give a correct answer to "is this detachable?"
And yeah, I think changing it to "transferable", while keeping the check, would be perfect. That reflects the actual use case.
(Also, it's strange that we assert instead of throw for detached array buffers in "transfer". I just checked all usages and it seems like it'd be safe, and perhaps an improvement, to update the algorithm to throw instead. But we can do that separately.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay - updated algorithm name to "transferable".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and updated PR title and description
As promised, the local algorithm "detachable" migrated to WebIDL[1], so we can drop our copy of the algorithm and reference WebIDL's version instead, which is identical except for a name change[3]. (If you're building the spec locally and run into errors, force a Bikeshed update[2] to ensure you have the latest indexes.) 1: whatwg/webidl@d6927d5 2: https://speced.github.io/bikeshed/#updating-bikeshed 3: whatwg/webidl#1419 (comment) Ref: webmachinelearning#351
As promised, the local algorithm "detachable" migrated to WebIDL[1], so we can drop our copy of the algorithm and reference WebIDL's version instead, which is identical except for a name change[3]. (If you're building the spec locally and run into errors, force a Bikeshed update[2] to ensure you have the latest indexes.) 1: whatwg/webidl@d6927d5 2: https://speced.github.io/bikeshed/#updating-bikeshed 3: whatwg/webidl#1419 (comment) Ref: #351
The new "transferable" algorithm allows an invoker to determine if a BufferSource can be detached without errors before invoking transfer. This is particularly helpful if several buffers are processed in a batch, as the validity of the operation can be determined up front, rather than during processing which could leave the buffers in a mix of states.
See discussion in [1]. A local copy of the algorithm was landed as [2] (as "detachable") but this seems like a useful addition to WebIDL.
1: webmachinelearning/webnn#351
2: https://webmachinelearning.github.io/webnn/#buffersource-detachable
Eliding the checklist since this is a helper algorithm not new functionality, but please ask if more details are desired
Preview | Diff