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.

Sign up for GitHub

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

Jump to bottom

Fake transfers for getMappedRange-created ArrayBuffers? #2072

Closed
kainino0x opened this issue Aug 27, 2021 · 11 comments · Fixed by #3098
Closed

Fake transfers for getMappedRange-created ArrayBuffers? #2072

kainino0x opened this issue Aug 27, 2021 · 11 comments · Fixed by #3098
Assignees
@kainino0x
Labels
copyediting Pure editorial stuff (copyediting, *.bs file syntax, etc.)
Milestone
Milestone 0

Comments

@kainino0x
Copy link
Contributor

Originally posted by @kainino0x in #747 (comment)

  • Non-transferable ArrayBuffers (mapped ArrayBuffers shouldn't be transferable to other threads). (We can do this without JS changes by faking transfers, by making them actually copies instead.)

We might be able to make this work entirely within our spec instead of having to go change the behavior of ArrayBuffer. In more detail:

If you tried to transfer a getMappedRange-created ArrayBuffer, it would appear to work entirely normally - old ArrayBuffer would get detached - except the underlying data would get copied into a new, run-of-the-mill ArrayBuffer, which means that any writes to the new ArrayBuffer would not appear in the GPUBuffer upon unmap().
@kainino0x kainino0x added this to the MVP milestone Aug 27, 2021
@kainino0x kainino0x mentioned this issue Aug 27, 2021
Open
7 tasks
@kainino0x
Copy link
Contributor Author

In other words, postMessage creates a new ArrayBuffer with a copy of the contents, and transfers that in place of the original one, and detaches the original ArrayBuffer and frees its underlying reference to the mapped data.

An alternative to this, which came up while discussing this with @pythonesque, would be to require these ArrayBuffers to be disposed somehow before .unmap() is called. Unmap would then fail (refuse to unmap) if this wasn't the case - there could be dangling ArrayBuffers somewhere out there referring to the mapped data. This could be done without making the current API worse: any ArrayBuffers that haven't been transferred away would still be on the same thread so they could be detached without problems.

If we had a way to share mappings across threads (detachable SharedArrayBuffers, #747), this would also work nicely here: The SharedArrayBuffer must be returned for disposal on every thread where it exists. If detachable SharedArrayBuffers existed at all I think they would probably have to work like this - i.e. each detachable SharedArrayBuffer would be a refcounted pointer to underlying data that would be freed after all references are dropped. And in unmap we would require the refcount to be 0.

A caveat of this approach is that, IIRC, when you transfer an object in JS it can actually arrive in multiple places, for reasons (relating to WebExtensions..?) that I don't completely understand. So we'd have to understand that.

@pythonesque
Copy link

Using SharedArrayBuffers would also be a considerable hit from an optimization perspective, I suspect (whether in JavaScript or not). In Rust, it would require treating each access to an element as an atomic which disables lots of useful optimizations and is generally very unpleasant to use (for example, safe transmute operations that treat a bag of bytes as a bag of plain old data types of other sizes or structures are not generally sound for atomics).

@kainino0x
Copy link
Contributor Author

JavaScript provides atomics, but allows racy access to SharedArrayBuffer without them.

I don't think we would change all mappings to return SharedArrayBuffer, it would probably be opt-in.

@pythonesque
Copy link

I think that if you allow racy accesses at all, the optimizer must still treat them as something like "Relaxed" or "unordered" LLVM intrinsics in order to ensure there are no unsafe optimizations (such as assuming an in-bounds index stored and then loaded from an array slot is still in bounds). Similarly, I think if there are any APIs that allow returning non-SharedArrayBuffer mappings, then we still need one of the other proposed solutions to figure out the unmap story. Of course if that's done, then opt-in SharedArrayBuffer could be nice :)

@kainino0x
Copy link
Contributor Author

Originally posted by @pythonesque in #747 (comment)

AFAIK, what you're describing is the behavior for non-transferable objects, though? Minus the fake detach, that is. I guess the point is that this wouldn't be an observable behavior change compared to the usual behavior (except in the context of WebGPU memory mapping, which isn't specified by TC39)?

Exactly. The only surprising thing would be that writes to the new ArrayBuffer wouldn't propagate to the GPUBuffer. Which isn't that weird, because the ArrayBuffer is no longer on the thread that issues the unmap() (which propagates the changes) anyway.

@pythonesque
Copy link

Exactly. The only surprising thing would be that writes to the new ArrayBuffer wouldn't propagate to the GPUBuffer. Which isn't that weird, because the ArrayBuffer is no longer on the thread that issues the unmap() (which propagates the changes) anyway.

Well, I suspect "isn't that weird if you know how unmap is implemented" doesn't mean it wouldn't be confusing to people... but at least as a stopgap solution it seems fine, and it's certainly better than crashing the process.

@m-schuetz
Copy link

m-schuetz commented Aug 28, 2021 •
edited
Loading

Instead of doing this silently, could you print a one-time warning to the console when a transfer does a copy instead? This seems like something you'd implement to reduce copies, but then you'd end up copying things anyway without knowing.

@kainino0x
Copy link
Contributor Author

kainino0x commented Aug 30, 2021 •
edited
Loading

Yes, it could definitely come with a console warning (advising you to make a copy to get rid of the warning).

@kainino0x kainino0x added the tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue label Jan 14, 2022
@kainino0x
Copy link
Contributor Author

Editors agreed to go with this direction.

@kainino0x kainino0x added tacit resolution queue Editors have agreed and intend to land if no feedback is given and removed tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue labels Feb 18, 2022
@kainino0x kainino0x mentioned this issue Mar 2, 2022
Closed
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 3, 2022
@austinEng
Implement copy-on-transfer semantics for GPUMappedArrayBuffer
2d525d2 
See gpuweb/gpuweb#2072 (comment)
Passes updated tests in gpuweb/cts#1027

Bug: 1243842
Change-Id: I3024b0185b4bdff3daf2ed32ab0f7286a4dbd1ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3496685
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>
Cr-Commit-Position: refs/heads/main@{#976902}
@kdashg kdashg removed the tacit resolution queue Editors have agreed and intend to land if no feedback is given label Mar 3, 2022
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 3, 2022
@austinEng
Revert "Implement copy-on-transfer semantics for GPUMappedArrayBuffer"
7146831 
This reverts commit 2d525d2.

Reason for revert: WebGPUExpectations update needs to re-run generator.

Original change's description:
> Implement copy-on-transfer semantics for GPUMappedArrayBuffer
>
> See gpuweb/gpuweb#2072 (comment)
> Passes updated tests in gpuweb/cts#1027
>
> Bug: 1243842
> Change-Id: I3024b0185b4bdff3daf2ed32ab0f7286a4dbd1ff
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3496685
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> Reviewed-by: Kai Ninomiya <kainino@chromium.org>
> Commit-Queue: Austin Eng <enga@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#976902}

Bug: 1243842
Change-Id: I20459fce5efe4987040f5e3dd9cc1b4338acd27f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3501104
Auto-Submit: Austin Eng <enga@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Owners-Override: Nidhi Jaju <nidhijaju@chromium.org>
Cr-Commit-Position: refs/heads/main@{#976943}
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 3, 2022
@austinEng
Reland "Implement copy-on-transfer semantics for GPUMappedArrayBuffer"
68365a9 
This reverts commit 7146831.

Reason for revert: Fixed instead by https://chromium-review.googlesource.com/c/chromium/src/+/3499755

Original change's description:
> Revert "Implement copy-on-transfer semantics for GPUMappedArrayBuffer"
>
> This reverts commit 2d525d2.
>
> Reason for revert: WebGPUExpectations update needs to re-run generator.
>
> Original change's description:
> > Implement copy-on-transfer semantics for GPUMappedArrayBuffer
> >
> > See gpuweb/gpuweb#2072 (comment)
> > Passes updated tests in gpuweb/cts#1027
> >
> > Bug: 1243842
> > Change-Id: I3024b0185b4bdff3daf2ed32ab0f7286a4dbd1ff
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3496685
> > Reviewed-by: Kentaro Hara <haraken@chromium.org>
> > Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> > Reviewed-by: Kai Ninomiya <kainino@chromium.org>
> > Commit-Queue: Austin Eng <enga@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#976902}
>
> Bug: 1243842
> Change-Id: I20459fce5efe4987040f5e3dd9cc1b4338acd27f
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3501104
> Auto-Submit: Austin Eng <enga@chromium.org>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
> Owners-Override: Nidhi Jaju <nidhijaju@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#976943}

Bug: 1243842
Change-Id: Ie124d05f59c330f8f5a4b1ba223d9608bc786cce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3500883
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Cr-Commit-Position: refs/heads/main@{#976959}
@kainino0x kainino0x added the copyediting Pure editorial stuff (copyediting, *.bs file syntax, etc.) label May 31, 2022
kainino0x added a commit to kainino0x/gpuweb that referenced this issue Jun 24, 2022
@kainino0x
"Fake" transfers of getMappedRange ArrayBuffers
6be4080 
Fixes gpuweb#2072
kainino0x added a commit to kainino0x/gpuweb that referenced this issue Jun 24, 2022
@kainino0x
"Fake" transfers of getMappedRange ArrayBuffers
494314b 
Fixes gpuweb#2072
kainino0x added a commit to kainino0x/gpuweb that referenced this issue Jun 24, 2022
@kainino0x
"Fake" transfers of getMappedRange ArrayBuffers
aee87a7 
Fixes gpuweb#2072
kainino0x added a commit to kainino0x/gpuweb that referenced this issue Jun 24, 2022
@kainino0x
"Fake" transfers of getMappedRange ArrayBuffers
2dcba0f 
Fixes gpuweb#2072
@kainino0x kainino0x mentioned this issue Jun 24, 2022
Merged
@kainino0x kainino0x self-assigned this Jun 24, 2022
toji pushed a commit that referenced this issue Jun 24, 2022
@kainino0x @toji
"Fake" transfers of getMappedRange ArrayBuffers
66d790a 
Fixes #2072
This was referenced Jul 2, 2022
Closed
Closed
jdarpinian pushed a commit to jdarpinian/gpuweb that referenced this issue Aug 12, 2022
@kainino0x @jdarpinian
"Fake" transfers of getMappedRange ArrayBuffers
9e7647b 
Fixes gpuweb#2072
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
@austinEng @copybara-github
Implement copy-on-transfer semantics for GPUMappedArrayBuffer
2167b6b 
See gpuweb/gpuweb#2072 (comment)
Passes updated tests in gpuweb/cts#1027

Bug: 1243842
Change-Id: I3024b0185b4bdff3daf2ed32ab0f7286a4dbd1ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3496685
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>
Cr-Commit-Position: refs/heads/main@{#976902}
NOKEYCHECK=True
GitOrigin-RevId: 2d525d281fa5c1a234baf36310e303cb9c54b697
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
@austinEng @copybara-github
Revert "Implement copy-on-transfer semantics for GPUMappedArrayBuffer"
fe57f72 
This reverts commit 2d525d281fa5c1a234baf36310e303cb9c54b697.

Reason for revert: WebGPUExpectations update needs to re-run generator.

Original change's description:
> Implement copy-on-transfer semantics for GPUMappedArrayBuffer
>
> See gpuweb/gpuweb#2072 (comment)
> Passes updated tests in gpuweb/cts#1027
>
> Bug: 1243842
> Change-Id: I3024b0185b4bdff3daf2ed32ab0f7286a4dbd1ff
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3496685
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> Reviewed-by: Kai Ninomiya <kainino@chromium.org>
> Commit-Queue: Austin Eng <enga@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#976902}

Bug: 1243842
Change-Id: I20459fce5efe4987040f5e3dd9cc1b4338acd27f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3501104
Auto-Submit: Austin Eng <enga@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Owners-Override: Nidhi Jaju <nidhijaju@chromium.org>
Cr-Commit-Position: refs/heads/main@{#976943}
NOKEYCHECK=True
GitOrigin-RevId: 714683105559d161e173df8420a19c9cb2cecb1f
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
@austinEng @copybara-github
Reland "Implement copy-on-transfer semantics for GPUMappedArrayBuffer"
ad4f784 
This reverts commit 714683105559d161e173df8420a19c9cb2cecb1f.

Reason for revert: Fixed instead by https://chromium-review.googlesource.com/c/chromium/src/+/3499755

Original change's description:
> Revert "Implement copy-on-transfer semantics for GPUMappedArrayBuffer"
>
> This reverts commit 2d525d281fa5c1a234baf36310e303cb9c54b697.
>
> Reason for revert: WebGPUExpectations update needs to re-run generator.
>
> Original change's description:
> > Implement copy-on-transfer semantics for GPUMappedArrayBuffer
> >
> > See gpuweb/gpuweb#2072 (comment)
> > Passes updated tests in gpuweb/cts#1027
> >
> > Bug: 1243842
> > Change-Id: I3024b0185b4bdff3daf2ed32ab0f7286a4dbd1ff
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3496685
> > Reviewed-by: Kentaro Hara <haraken@chromium.org>
> > Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> > Reviewed-by: Kai Ninomiya <kainino@chromium.org>
> > Commit-Queue: Austin Eng <enga@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#976902}
>
> Bug: 1243842
> Change-Id: I20459fce5efe4987040f5e3dd9cc1b4338acd27f
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3501104
> Auto-Submit: Austin Eng <enga@chromium.org>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
> Owners-Override: Nidhi Jaju <nidhijaju@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#976943}

Bug: 1243842
Change-Id: Ie124d05f59c330f8f5a4b1ba223d9608bc786cce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3500883
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Cr-Commit-Position: refs/heads/main@{#976959}
NOKEYCHECK=True
GitOrigin-RevId: 68365a9dc0a62413d5f2c2a96f926ef540ee6ee2
@erights
Copy link

erights commented Dec 23, 2024

Please see https://github.com/tc39/proposal-immutable-arraybuffer which proposes Immutable ArrayBuffers, is at Stage 2 of the tc39 process, has enthusiastic support from some implementations, and not (yet?) any serious objections. How applicable would this be to the concerns expressed above?

@kainino0x
Copy link
Contributor Author

^ I've replied on #747 (comment) (the need for read-only ArrayBuffers there is the same root issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kainino0x kainino0x

Labels
copyediting Pure editorial stuff (copyediting, *.bs file syntax, etc.)
Projects
None yet
Milestone
Milestone 0
Development

Successfully merging a pull request may close this issue.

"Fake" transfers of getMappedRange ArrayBuffers kainino0x/gpuweb
5 participants
@erights @kainino0x @pythonesque @kdashg @m-schuetz