Xrays support for SharedArrayBuffer

RESOLVED FIXED in Firefox 52

Status

()

Core
XPConnect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: lth, Assigned: evilpie)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla52
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
I'm not actually sure what's needed here but after watching bholley's talk I'm pretty sure that SAB/STA will be opaque to Xrays clients but need not be and should not be.
(In reply to Lars T Hansen [:lth] from comment #0)
> I'm not actually sure what's needed here but after watching bholley's talk
> I'm pretty sure that SAB/STA will be opaque to Xrays clients

Correct.

> but need not be and should not be.

Can you elaborate a bit on the use cases here?

Note that in my implementation for TypedArray Xrays, I deliberately made indexed access throw, because accessing a TypedArray that way is going to be exceedingly slow.
(Reporter)

Comment 2

3 years ago
Use cases for Xrays are unclear to me - I'm new to that problem space.  Probably the use cases are analogous to those of TypedArray, certainly initially.  SharedArrayBuffer and SharedTypedArray currently behave much like ArrayBuffer and TypedArray, except that STA is not neuterable.

(The shared memory feature is described here in the event you've not seen it:
https://docs.google.com/document/d/1NDGA_gZJ7M7w1Bh8S0AoDyEqwDdRh4uSoTPSNn77PFk/edit?usp=sharing)
We don't have Xrays to ArrayBuffer, though you could certainly add them - it would be relatively straightforward, though I'm not aware of any use-cases it supports.

Implementing xrays to STA would probably just involve doing the analogous thing to what we do for TypedArray. This basically means providing safe access to all the simple properties, and having indexed access throw with a message saying ("This will be really slow! Use Cu.cloneInto instead!").

If you don't have support for this stuff in the structured clone algorithm, _that_ would be something worth doing.
(Reporter)

Comment 4

3 years ago
SAB and STA are supported in structured clone, and that's pretty well tested at this point.  The only things not landed are the DOM bits for blocking wait (bug 1074237), and now modest Xray support.
(In reply to Lars T Hansen [:lth] from comment #4)
> SAB and STA are supported in structured clone, and that's pretty well tested
> at this point.  The only things not landed are the DOM bits for blocking
> wait (bug 1074237), and now modest Xray support.

Personally I don't think that Xray support here is a blocker, though I would certainly not object to adding it. Let me know if you want to and I can point you in the right direction.
(Assignee)

Updated

a year ago
Assignee: nobody → evilpies
Blocks: 914970
(Assignee)

Comment 6

a year ago
SharedTypedArrays are gone, do we really need Xray support for Atomis or is SharedTypedArray good enough?
(Reporter)

Comment 7

a year ago
I seriously doubt this is interesting for Atomics.  SharedArrayBuffer should have whatever support ArrayBuffer has, however.
(Reporter)

Updated

a year ago
Summary: Xrays support for SharedArrayBuffer, SharedTypedArray, and maybe the Atomics object → Xrays support for SharedArrayBuffer
(Assignee)

Comment 8

a year ago
Created attachment 8805200 [details] [diff] [review]
Actually support creating TypedArrays with Xray SharedArrayBuffer

Calling the native function that is supposed to create the typed array with the shared array buffer would fail the "this" test. I don't think that ever worked before.
(Assignee)

Comment 9

a year ago
Created attachment 8805201 [details] [diff] [review]
Change SharedArrayBuffer to use ClassSpec
(Assignee)

Comment 10

a year ago
Created attachment 8805203 [details] [diff] [review]
Add SharedArrayBuffer to JSXray

I almost forgot about the versioning here, but we only enable SharedArrayBuffer on Aurora/Nightly at the moment. If we never want to implement SharedArrayBuffer.prototype.slice I could remove that check.
(Assignee)

Comment 11

a year ago
Bz, can you review this please?
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 12

a year ago
(In reply to Tom Schuster [:evilpie] from comment #10)
> Created attachment 8805203 [details] [diff] [review]
> Add SharedArrayBuffer to JSXray
> 
> I almost forgot about the versioning here, but we only enable
> SharedArrayBuffer on Aurora/Nightly at the moment. If we never want to
> implement SharedArrayBuffer.prototype.slice I could remove that check.

SAB.prototype.slice is definitely coming, probably in FF52.

The *tentative* plan is to enable SAB in FF51 with the feature set that is currently in m-c and is in the process of being uplifted to Aurora.
Comment on attachment 8805200 [details] [diff] [review]
Actually support creating TypedArrays with Xray SharedArrayBuffer

r=me
Attachment #8805200 - Flags: review+
Comment on attachment 8805201 [details] [diff] [review]
Change SharedArrayBuffer to use ClassSpec

r=me
Attachment #8805201 - Flags: review+
Comment on attachment 8805203 [details] [diff] [review]
Add SharedArrayBuffer to JSXray

>+  if (!isReleaseOrBeta) {

Else, can you have this test assert that 'SharedArrayBuffer' is not a thing, so we don't forget to update the test when we ship it on release/beta?

>+      is(t.byteLength, 12, "ArrayBuffer byteLength is correct");

I'd appreciate it if the third arg here became `${c} byteLength is correct`, and similar for the other test descriptions in this function.

r=me with those fixed.
Flags: needinfo?(bzbarsky)
Attachment #8805203 - Flags: review+

Comment 16

a year ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d27de07adaae
Actually support creating TypedArrays with Xray SharedArrayBuffer. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9e245f787d
Change SharedArrayBuffer to use ClassSpec. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/40cd4eb145ff
Add SharedArrayBuffer to JSXray. r=bz

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d27de07adaae
https://hg.mozilla.org/mozilla-central/rev/0b9e245f787d
https://hg.mozilla.org/mozilla-central/rev/40cd4eb145ff
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.