Implement shared WebAssembly.Memory objects

RESOLVED FIXED in Firefox 59

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(6 attachments, 8 obsolete attachments)

16.74 KB, patch
luke
: review+
Details | Diff | Splinter Review
9.26 KB, patch
luke
: review+
Details | Diff | Splinter Review
8.62 KB, patch
luke
: review+
Details | Diff | Splinter Review
12.28 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.55 KB, patch
luke
: review+
Details | Diff | Splinter Review
51.21 KB, patch
luke
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
This came in with the threads proposal (https://github.com/WebAssembly/threads).

This includes at least these work items:

- recognizing and generating the "shared" attribute in the textual form
- extending import/export/verification to deal with the shared attribute
- initializing objects with shared memory
- extending the grow_memory / current_memory opcodes to shared memory
- structured-cloning the new memory objects

The new atomic operations and verification of those against shared memory is handled by bug 1377576 however.
(Assignee)

Updated

a year ago
Depends on: 1389471
(Assignee)

Updated

a year ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Priority: -- → P2
(Assignee)

Updated

a year ago
Priority: P2 → P1
(Assignee)

Comment 1

a year ago
Created attachment 8925895 [details] [diff] [review]
bug1389464-refactor-mmap-code.patch

Factor the mmapping code so that SharedArrayRawBuffer and WasmArrayRawBuffer can share it.  SARB will use more of this code in a subsequent patch for growing memory.

There is a side effect here - the accounting of max number of concurrently live mappings implemented for SARB is now also implemented for Wasm.  This should be so because that accounting had nothing to do with shared memory per se, but with the number of mappings, and Wasm is just as sensitive to too many of those as SARB is.
Attachment #8925895 - Flags: review?(luke)
(Assignee)

Comment 2

a year ago
Created attachment 8925896 [details] [diff] [review]
bug1389464-preparatory-cleanup.patch

Clean up things that are in need of cleaning up (crufty code and rotten comments).  I've factored this into a patch so that it doesn't confuse subsequent patches.
Attachment #8925896 - Flags: review?(luke)
(Assignee)

Comment 3

a year ago
Created attachment 8925897 [details] [diff] [review]
bug1389464-parse-shmem-attributes.patch

Parse the 'shared' attribute in the text format, and generate it when generating text.
Attachment #8925897 - Flags: review?(luke)
(Assignee)

Comment 4

a year ago
Created attachment 8925898 [details] [diff] [review]
bug1389464-wasm-shared-memory-object.patch

This is the main patch implementing shared memory.  It is f? only for now since the reading of the length field is not controlled by a lock; the cases that need to be handled are easily findable because the getter of the length value is called 'volatileMemoryLength'.

Rather than trying to merge WasmArrayRawBuffer and SharedArrayRawBuffer I've added extra fields to SARB to hold Wasm data, and there are therefore different code paths for the two cases once we get far down in the call tree.  This keeps complexity down.
Attachment #8925898 - Flags: feedback?(luke)
(Assignee)

Comment 5

a year ago
Created attachment 8925899 [details] [diff] [review]
bug1389464-wasm-shared-memory-tests.patch

Test cases for wasm shared memory.  (The test cases for atomics are attached to bug 1377562, and the test cases for cloning are attached to bug 1412852.)
Attachment #8925899 - Flags: review?(luke)
(Assignee)

Comment 6

a year ago
Created attachment 8926422 [details] [diff] [review]
bug1389464-always-lock-sarb.patch

So, this is code to always lock access to the SharedArrayRawBuffer to read its length.  Recall, the point of this is to make sure that the committed pages and the observed length are always in sync.

Two parts to this, one is to make all the memoryLength accessors lock, and the second is to make this work reliably with the signal handler.

In principle the signal handler wants to try to take the SARB's lock, and if that fails, to retry the access (since memory is in the process of growing and maybe it'll be fine the second time).  This fails because (a) we do not have a trylock() operation and (b) even if we did - no reason we can't - it wouldn't be legal to use it from the signal handler (pthread restriction).

Instead this patch introduces a spinlock which is taken in the signal handler, and also taken in the critical section of the lock elsewhere, to coordinate the access.

I'm asking for feedback, not review, and I've not rolled this patch into the other one yet because I wanted to have a discussion about the approach, but LMK if a rolled-up patch would be best.
Attachment #8926422 - Flags: feedback?(luke)
(Assignee)

Comment 7

a year ago
Created attachment 8927295 [details] [diff] [review]
bug1389464-wasm-shared-memory-object-v2.patch

Wasm shared memory, main patch.  See comment 4, this has some minor bug fixes.
Attachment #8925898 - Attachment is obsolete: true
Attachment #8925898 - Flags: feedback?(luke)
Attachment #8927295 - Flags: feedback?(luke)
(Assignee)

Comment 8

a year ago
Created attachment 8927296 [details] [diff] [review]
bug1389464-always-lock-sarb-v2.patch

Lock for access to length, updated with minor bug fixes.  See comment 6.
Attachment #8926422 - Attachment is obsolete: true
Attachment #8926422 - Flags: feedback?(luke)
Attachment #8927296 - Flags: feedback?(luke)
(Assignee)

Comment 9

a year ago
Created attachment 8927759 [details] [diff] [review]
bug1389464-parse-shmem-attributes-v2.patch

Parse shared memory attributes, see comment 3.  Updated with proper #ifdeffery.
Attachment #8925897 - Attachment is obsolete: true
Attachment #8925897 - Flags: review?(luke)
Attachment #8927759 - Flags: review?(luke)
(In reply to Lars T Hansen [:lth] from comment #1)
> There is a side effect here - the accounting of max number of concurrently
> live mappings implemented for SARB is now also implemented for Wasm.

Excellent, I was just thinking, after last CG discussion on this issue, that we should do this.
Comment on attachment 8925895 [details] [diff] [review]
bug1389464-refactor-mmap-code.patch

Review of attachment 8925895 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ArrayBufferObject.cpp
@@ +123,5 @@
> +            return nullptr;
> +        }
> +    }
> +
> +# ifdef XP_WIN

nit: this #ifdef isn't inside any enclosing #if so no need to "indent" with spaces between # and "ifdef" (here and below)

::: js/src/vm/ArrayBufferObject.h
@@ +48,5 @@
> +// the mapping, and `mappedSize` the size of that mapping.
> +void UnmapBufferMemory(void* dataStart, size_t mappedSize);
> +
> +// Return the number of currently live mapped buffers.
> +int32_t LiveMappedBufferCount();

Could we add a MOZ_ASSERT_IF(!JSRuntime::hasLiveRuntimes(), !wasm::LiveMappedBufferCount()) in JS_ShutDown()?  This matches existing asserts in jit::ReleaseProcessExecutableMemory() that all jit code has been released.
Attachment #8925895 - Flags: review?(luke) → review+
Comment on attachment 8925896 [details] [diff] [review]
bug1389464-preparatory-cleanup.patch

Review of attachment 8925896 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/lib/wasm.js
@@ +8,5 @@
> +        wasmTextToBinary(`(module (memory 1 1 shared)
> +                           (func (result i32)
> +                            (i32.atomic.load (i32.const 0)))
> +                           (export "" 0))`);
> +        return true;

It'd be good to WebAssembly.validate() the results in case we conditionalize the feature at validation but don't bother with wasmTextToBinary().
Attachment #8925896 - Flags: review?(luke) → review+
Comment on attachment 8925899 [details] [diff] [review]
bug1389464-wasm-shared-memory-tests.patch

Review of attachment 8925899 [details] [diff] [review]:
-----------------------------------------------------------------

Nice test

::: js/src/jit-test/tests/wasm/memory-sharing.js
@@ +54,5 @@
> +			      0x05,                   // Memory
> +			      0x03,                   // Section size
> +			      0x01,                   // One memory
> +			      0x02,		      // Shared, min only (illegal)
> +			      0x01]);		      // Min

Straight to the metal with this one ;)
Attachment #8925899 - Flags: review?(luke) → review+
Comment on attachment 8927759 [details] [diff] [review]
bug1389464-parse-shmem-attributes-v2.patch

Review of attachment 8927759 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmJS.cpp
@@ +482,5 @@
> +            return false;
> +        RootedId sharedId(cx, AtomToId(sharedAtom));
> +
> +        bool foundShared;
> +        if (HasProperty(cx, obj, sharedId, &foundShared) && foundShared) {

If HasProperty() fails, you need to immediately 'return false' to propagate the error.

@@ +1387,5 @@
>  
>      RootedObject obj(cx, &args[0].toObject());
>      Limits limits;
> +    if (!GetLimits(cx, obj, MaxMemoryInitialPages, MaxMemoryMaximumPages, "Memory", &limits,
> +                   /*allowShared=*/true))

For one-off cases, the inline /*foo=*/true style makes sense, but when there are 10 or so uses, I think it'd be nice to have in WasmTypes.h some `enum class Shareable { True, False }` used for both all these {Get,Decode,Parse}Limits() parameters as well as the 'shared' field of Limits and Limits ctor parameter.

In retrospect (and not for this patch unless you want), the ad hoc trio of fields {memoryUsage, minMemoryLength, maxMemoryLength} in ModuleEnvironment and Metadata should've been a Maybe<Limits> (with MemoryUsage going away), later to be replaced by Vector<Limits> when we/if we get multi-memories (analogous to TableDescVector).

::: js/src/wasm/WasmTextToBinary.cpp
@@ +2812,5 @@
>  
> +    bool isSharedMemory = false;
> +    if (c.ts.getIf(WasmToken::Shared, &token)) {
> +        // A missing maximum can alternatively be caught later, but the early
> +        // syntax error seems desirable.

In other cases, we've been pretty permissive in WasmTextToBinary (allowing, e.g., out-of-bounds indices) which allows more expressive testing.

::: js/src/wasm/WasmValidate.cpp
@@ +874,5 @@
>      uint8_t flags;
>      if (!d.readFixedU8(&flags))
>          return d.fail("expected flags");
>  
> +    uint8_t mask = allowShared ? 0x3 : 0x1;

Could you give these symbolic names in WasmBinaryConstants.h and use here and below and in WasmTextToBinary.cpp?
Attachment #8927759 - Flags: review?(luke) → review+
Comment on attachment 8927295 [details] [diff] [review]
bug1389464-wasm-shared-memory-object-v2.patch

Review of attachment 8927295 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looks good.

::: js/src/vm/ArrayBufferObject.cpp
@@ +756,5 @@
>      uint64_t mappedSizeWithHeader = mappedSize + gc::SystemPageSize();
>      uint64_t numBytesWithHeader = numBytes + gc::SystemPageSize();
>  
> +    if (!ArrayBufferObject::admissibleLength(numBytesWithHeader))
> +        return nullptr;

Along the lines of the other admissibleLength() comment, it seems like this wouldn't be necessary.

::: js/src/vm/ArrayBufferObject.h
@@ +373,5 @@
> +#define ROUND_UP(v, a) ((v) % (a) == 0 ? (v) : v + a - ((v) % (a)))
> +
> +    template <typename T>
> +    static T* allocateWasm(JSContext* cx, uint32_t initialSize, const Maybe<uint32_t>& maxSize,
> +                           T* (*tryAlloc)(uint32_t, const Maybe<uint32_t>&))

I was wondering if there was a more natural way to share code bewteen AB and SAB than these two methods.  I noticed that there are two sites (Memory ctor, Module::instantiateMemory) that already branch on limits.shared to call either AB or SAB's createForWasm, so what if we could have a single entry point:
  template <class ObjT, class RawBufT> bool CreateBuffer(cx, Limits, MutableHandleArrayBufferObjectMaybeShared)
that used RawBufT::Allocate (instead of the tryAlloc func ptr) and puts the implementation in the .cpp, using explicit template instantiation.

::: js/src/vm/SharedArrayObject.cpp
@@ +44,5 @@
>      return allocSize + wasm::GuardSize;
>  #endif
>  }
>  
> +// maxSize does not include the header page.

nit: taking 'maxSize' to be the wasm declared maximum size, then it follows that this wouldn't include the header page.

@@ +70,5 @@
>      // so guard against it on principle.
>      MOZ_ASSERT(length != (uint32_t)-1);
>  
> +    bool preparedForAsmJS = jit::JitOptions.asmJSAtomicsEnable && IsValidAsmJSHeapLength(length);
> +    bool preparedForWasm = max.isSome();

Just to make sure: this means a wasm-generated SAB can be linked to asm.js, right?

@@ +89,5 @@
> +
> +    uint64_t mappedSizeWithHeader = mappedSize + gc::SystemPageSize();
> +    uint64_t allocSizeWithHeader = allocSize + gc::SystemPageSize();
> +
> +    if (!ArrayBufferObject::admissibleLength(allocSizeWithHeader))

Although conservative, couldn't we check admissibility on 'length' instead?  Actually, shouldn't this be something we can MOZ_ASSERT() on entry, given that all the callers should've already checked and reported an explicit length-too-big error?  The 3 callers I know of (SAB ctor, Memory ctor, wasm memory def) are all checked against INT32_MAX or a more-conservative limit (MaxMemoryInitialPages).

::: js/src/wasm/WasmGenerator.cpp
@@ +833,4 @@
>      metadata_->memoryUsage = env_->memoryUsage;
>      metadata_->minMemoryLength = env_->minMemoryLength;
>      metadata_->maxMemoryLength = env_->maxMemoryLength;
> +    metadata_->memoryUsage = env_->memoryUsage;

Looks like this line is a dup.

::: js/src/wasm/WasmJS.cpp
@@ +1425,5 @@
> +    RootedArrayBufferObjectMaybeShared buffer(cx, &memoryObj->buffer());
> +
> +    if (memoryObj->isShared()) {
> +        uint32_t memoryLength = memoryObj->volatileMemoryLength();
> +        if (memoryLength > buffer->byteLength()) {

nit: can you assert (memoryLength >= buffer->byteLength()?

::: js/src/wasm/WasmModule.cpp
@@ +926,1 @@
>          if (!memory)

nit: can you remove \n between `memory.set()` and `if (!memory)`?
Attachment #8927295 - Flags: feedback?(luke) → feedback+
Comment on attachment 8927296 [details] [diff] [review]
bug1389464-always-lock-sarb-v2.patch

Review of attachment 8927296 [details] [diff] [review]:
-----------------------------------------------------------------

I just realized something about the whole signal handler path that uses the memory length: it's not for wasm at all, it's to support a pretty Insane Wolf asm.js optimization where we fold the offset into the asm.js load or store which means that, when you fault, it's possible you've actually gone so far out of bounds that you are now, by JS semantics, in bounds.  That's why we need to know the length: to see if you've wrapped around.  Notice how the !defined(WASM_HUGE_MEMORY) version of HandleMemoryAccess() doesn't look at memory length at all, it just redirects pc (when !defined(WASM_HUGE_MEMORY), asm.js never faults b/c it has no unaligned access and does no offset folding).  So rather than adding this subtle spinlock logic, let's just kill the asm.js constant-folding optimization.

We could *almost* just delete the opt and then delete all this code including the whole x64 decompiler, but there is one rub: we use the x64 decompiler to determine into which register to store the asm.js NaN or 0.
Attachment #8927296 - Flags: feedback?(luke)
(Assignee)

Comment 17

a year ago
(In reply to Luke Wagner [:luke] from comment #16)
> Comment on attachment 8927296 [details] [diff] [review]
> bug1389464-always-lock-sarb-v2.patch
> 
> Review of attachment 8927296 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just realized something about the whole signal handler path that uses the
> memory length: it's not for wasm at all, it's to support a pretty Insane
> Wolf asm.js optimization where we fold the offset into the asm.js load or
> store which means that, when you fault, it's possible you've actually gone
> so far out of bounds that you are now, by JS semantics, in bounds.  That's
> why we need to know the length: to see if you've wrapped around.  Notice how
> the !defined(WASM_HUGE_MEMORY) version of HandleMemoryAccess() doesn't look
> at memory length at all, it just redirects pc (when
> !defined(WASM_HUGE_MEMORY), asm.js never faults b/c it has no unaligned
> access and does no offset folding).  So rather than adding this subtle
> spinlock logic, let's just kill the asm.js constant-folding optimization.

OK, I'll look into that.  We'll still want to lock on length access normally, but avoiding it from the signal handler seems like a real win.
(Assignee)

Comment 18

a year ago
(In reply to Luke Wagner [:luke] from comment #15)
> Comment on attachment 8927295 [details] [diff] [review]
> bug1389464-wasm-shared-memory-object-v2.patch
> 
> Review of attachment 8927295 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> @@ +70,5 @@
> >      // so guard against it on principle.
> >      MOZ_ASSERT(length != (uint32_t)-1);
> >  
> > +    bool preparedForAsmJS = jit::JitOptions.asmJSAtomicsEnable && IsValidAsmJSHeapLength(length);
> > +    bool preparedForWasm = max.isSome();
> 
> Just to make sure: this means a wasm-generated SAB can be linked to asm.js,
> right?

That's the intent, though (a) I don't think I have any test cases yet that verify that yet (I will create some) and (b) further down in that same function there's a conditional that prioritizes prepping the buffer for wasm if both those flags are true.  Now, if a wasm buffer by construction is compatible with asm.js then we're fine but if not then there's a conflict here.  I believe that's true but I'll double check.

> @@ +89,5 @@
> > +
> > +    uint64_t mappedSizeWithHeader = mappedSize + gc::SystemPageSize();
> > +    uint64_t allocSizeWithHeader = allocSize + gc::SystemPageSize();
> > +
> > +    if (!ArrayBufferObject::admissibleLength(allocSizeWithHeader))
> 
> Although conservative, couldn't we check admissibility on 'length' instead?

Yes.
 
> Actually, shouldn't this be something we can MOZ_ASSERT() on entry, given
> that all the callers should've already checked and reported an explicit
> length-too-big error?  The 3 callers I know of (SAB ctor, Memory ctor, wasm
> memory def) are all checked against INT32_MAX or a more-conservative limit
> (MaxMemoryInitialPages).

A MOZ_ASSERT (really a MOZ_RELEASE_ASSERT given how costly the function is) is fine with me both here and elsewhere, I just became really paranoid since there are many paths leading to the allocation functions and the INT32_MAX limit is not baked into the wasm spec.  It happens that MaxMemoryInitialPages is correct now (1GB) but defense in depth and all that.
(Assignee)

Comment 19

a year ago
(In reply to Lars T Hansen [:lth] from comment #18)
> (In reply to Luke Wagner [:luke] from comment #15)
> > Comment on attachment 8927295 [details] [diff] [review]
> > bug1389464-wasm-shared-memory-object-v2.patch
> > 
> > Review of attachment 8927295 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > @@ +70,5 @@
> > >      // so guard against it on principle.
> > >      MOZ_ASSERT(length != (uint32_t)-1);
> > >  
> > > +    bool preparedForAsmJS = jit::JitOptions.asmJSAtomicsEnable && IsValidAsmJSHeapLength(length);
> > > +    bool preparedForWasm = max.isSome();
> > 
> > Just to make sure: this means a wasm-generated SAB can be linked to asm.js,
> > right?
> 
> That's the intent, though (a) I don't think I have any test cases yet that
> verify that yet (I will create some) and (b) further down in that same
> function there's a conditional that prioritizes prepping the buffer for wasm
> if both those flags are true.  Now, if a wasm buffer by construction is
> compatible with asm.js then we're fine but if not then there's a conflict
> here.  I believe that's true but I'll double check.

There's an interesting case where we create a memory for wasm and then extract the buffer and link it to asm.js code, and *then* we grow the memory.  asm.js should not observe the heap growth (for any operation at all) but I sure don't know if that is true.  Most plausibly this has been vetted for non-shared memory so it might be fine.  On the other hand, if asm.js code depends on traps for range checking and more memory has been committed for wasm, then it might not hit those traps.

Anyway the attached test case hits an assert in CheckLimits.  Investigating further.
(Assignee)

Comment 20

a year ago
Created attachment 8929011 [details]
atom.js
(Assignee)

Comment 21

a year ago
Looks like the computation of preparedForAsmJS needs to be more subtle: it should only be true if there's a declared max and the declared max is the same as the length.  The same constraint shows up in the ArrayBufferObject code but in a different guise.
(Assignee)

Comment 22

a year ago
Created attachment 8929044 [details] [diff] [review]
bug1389464-wasm-asm-buffer-sharing-test.patch

Test case for sharing a SAB between wasm and asm.js.

This fell out of the test case earlier, and some adjustments to the code.

Does this behavior feel meaningful to you?  The presumption here is that if we have a fixed-length buffer (max == initial) then the buffer mapping is constant, and so there should be no problem with asm.js observing a grown buffer.

Arguably even if initial < max we could allow this if the current length has reached the max, I could look into that further.

The simple alternative is to just say that a wasm shared memory should never be used for asm.js; we can recognize a SAB that's been touched by wasm so there's no problem doing that either.  Given how we're deprecating asm.js atomics this may be just fine.
Attachment #8929044 - Flags: review?(luke)
(Assignee)

Comment 23

a year ago
(In reply to Luke Wagner [:luke] from comment #16)
> Comment on attachment 8927296 [details] [diff] [review]
> bug1389464-always-lock-sarb-v2.patch
> 
> Review of attachment 8927296 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just realized something about the whole signal handler path that uses the
> memory length: it's not for wasm at all, it's to support a pretty Insane
> Wolf asm.js optimization where we fold the offset into the asm.js load or
> store which means that, when you fault, it's possible you've actually gone
> so far out of bounds that you are now, by JS semantics, in bounds.  That's
> why we need to know the length: to see if you've wrapped around.  Notice how
> the !defined(WASM_HUGE_MEMORY) version of HandleMemoryAccess() doesn't look
> at memory length at all, it just redirects pc (when
> !defined(WASM_HUGE_MEMORY), asm.js never faults b/c it has no unaligned
> access and does no offset folding).  So rather than adding this subtle
> spinlock logic, let's just kill the asm.js constant-folding optimization.

Actually, I question whether we need to do anything at all here, because I'm pretty sure the memory cannot grow while the signal handler is executing.

The path that reads the length is for asm.js only.  If the memory is unshared there should be no problem; there cannot be concurrent growth.  If the memory is shared then it is either a normal SAB or a SAB from a Wasm memory.  If it is a normal SAB then it cannot grow.  If it is the SAB from a Wasm memory then there is a linking guard (as of yesterday, see preceding comments) that the initial length of the SAB must equal the max length (which is mandatory, it being wasm memory).  In this case the memory also cannot grow.

We might discuss this synchronously next week but in the mean time I'm going to not do anything rash.
Flags: needinfo?(luke)
(In reply to Lars T Hansen [:lth] from comment #22)
> The simple alternative is to just say that a wasm shared memory should never
> be used for asm.js; we can recognize a SAB that's been touched by wasm so
> there's no problem doing that either.  Given how we're deprecating asm.js
> atomics this may be just fine.

This was actually my inclination and the reason for my comment.  Even if we can't find a hard technical reason now to disallow, it doesn't mean we won't in the future so seems better to be conservative for now.  FWIW, the same restriction applies to the asm.js and the AB of a non-shared Memory.

(In reply to Lars T Hansen [:lth] from comment #23)
> Actually, I question whether we need to do anything at all here, because I'm
> pretty sure the memory cannot grow while the signal handler is executing.

Hah, you're right!  This keeps getting easier and easier :)
Flags: needinfo?(luke)
Comment on attachment 8929044 [details] [diff] [review]
bug1389464-wasm-asm-buffer-sharing-test.patch

Review of attachment 8929044 [details] [diff] [review]:
-----------------------------------------------------------------

If you agree with comment 24, then this test should fail (and probably a negative test case would be good to add instead).
Attachment #8929044 - Flags: review?(luke)
(Assignee)

Comment 26

a year ago
Created attachment 8930069 [details] [diff] [review]
bug1389464-wasm-shared-memory-object-v3.patch

Implement WebAssembly.Memory.  This is the same patch as before, but with comments on the previous patch addressed, with locking on access to the memory length, and a restriction that wasm shared memory can't be used for asm.js (tests follow in separate patch).

One minor note here, the structured cloning patch which is subsequent to this gets rid of two instances of locking / length access, since structured cloning is changed so as to transmit the SAB length too.
Attachment #8927295 - Attachment is obsolete: true
Attachment #8927296 - Attachment is obsolete: true
Attachment #8929011 - Attachment is obsolete: true
Attachment #8929044 - Attachment is obsolete: true
Attachment #8930069 - Flags: review?(luke)
(Assignee)

Comment 27

a year ago
Created attachment 8930070 [details] [diff] [review]
bug1389464-wasm-xor-asm-shmem-test.patch

Test cases to ensure that a SAB from wasm memory can't be used for asm.js.
Attachment #8930070 - Flags: review?(luke)
(Assignee)

Comment 28

a year ago
Comment on attachment 8930069 [details] [diff] [review]
bug1389464-wasm-shared-memory-object-v3.patch

Need to correct a misedit.
Attachment #8930069 - Flags: review?(luke)
(Assignee)

Comment 29

a year ago
Created attachment 8930088 [details] [diff] [review]
bug1389464-wasm-shared-memory-object-v4.patch

(Revised)

Implement WebAssembly.Memory.  This is the same patch as before, but with comments on the previous patch addressed, with locking on access to the memory length, and a restriction that wasm shared memory can't be used for asm.js (tests follow in separate patch).

One minor note here, the structured cloning patch which is subsequent to this gets rid of two instances of locking / length access, since structured cloning is changed so as to transmit the SAB length too.
Attachment #8930069 - Attachment is obsolete: true
Attachment #8930088 - Flags: review?(luke)
Comment on attachment 8930070 [details] [diff] [review]
bug1389464-wasm-xor-asm-shmem-test.patch

Review of attachment 8930070 [details] [diff] [review]:
-----------------------------------------------------------------

Heh, I like the comments in the test.
Attachment #8930070 - Flags: review?(luke) → review+
Comment on attachment 8930088 [details] [diff] [review]
bug1389464-wasm-shared-memory-object-v4.patch

Review of attachment 8930088 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!  R+ with nits and guard on the shared_memory pref in both DecodeMemoryLimits() and WasmJS.cpp's GetLimits().

::: js/src/vm/ArrayBufferObject.cpp
@@ +1333,5 @@
> +{
> +    // The length of an ArrayBuffer or SharedArrayBuffer can be at most
> +    // INT32_MAX. Note: if this hard limit changes, update the clamping behavior
> +    // in wasm::DecodeMemoryLimits.
> +    return length <= INT32_MAX;

Mostly pre-existing, but since it's unlikely that this INT32_MAX limit will decrease but rather that we will be tempted to increase the MaxMemoryInitialPages, it seems like this is the wrong place for the comment; it should be on MaxMemoryInitialPages.  But even better: instead of an isAdmissibleLength() function, could we have a 'static const size_t MaxBufferByteLength = INT32_MAX', check against that limit in all our asserts and static_assert(MaxMemoryInitialpages <= MaxBufferByteLength))?

::: js/src/vm/ArrayBufferObject.h
@@ +371,3 @@
>      static MOZ_MUST_USE bool prepareForAsmJS(JSContext* cx, Handle<ArrayBufferObject*> buffer,
>                                               bool needGuard);
> +    void acceptRawBuffer(JSContext* cx, WasmArrayRawBuffer* buffer, uint32_t byteLength);

nit: could this be named "initialize()" instead?

::: js/src/vm/SharedArrayObject.cpp
@@ +76,2 @@
>      uint32_t allocSize = SharedArrayAllocSize(length);
> +    if (allocSize < length)

nit: could "alloc" in this variable and function name be changed to "accessible"?  ("alloc" sounds like it's the size we actually passed to the allocate call which was confusing me.)

::: js/src/vm/SharedArrayObject.h
@@ +47,5 @@
> + * it may grow toward maxSize_.  See extensive comments above WasmArrayRawBuffer
> + * in ArrayBufferObject.cpp.
> + *
> + * length_ only grows when the lock is held, but it can be read without the lock
> + * being taken.

I think the "but it can be read without the lock being taken" is no longer true.

::: js/src/wasm/WasmJS.cpp
@@ +1314,5 @@
> +            buffer = newBuffer;
> +            memoryObj->setReservedSlot(BUFFER_SLOT, ObjectValue(*newBuffer));
> +        }
> +    }
> +    args.rval().setObject(*buffer);

nit: could you add a \n before the rval().setObject()?

::: js/src/wasm/WasmJS.h
@@ +225,5 @@
> +    // The current length of the memory.  In the case of shared memory, the
> +    // length can change at any time.  Also note that this will acquire a lock
> +    // for shared memory.
> +    //
> +    // DO NOT CALL THIS FROM A SIGNAL HANDLER.

nit: since we're no longer doing the special spin-lock dance, maybe we could downgrade this from scare-caps to be an uncapitalized clause combined with the preceding sentence with a ", so".
Attachment #8930088 - Flags: review?(luke) → review+
(Assignee)

Comment 32

a year ago
(In reply to Luke Wagner [:luke] from comment #31)
> Comment on attachment 8930088 [details] [diff] [review]
> bug1389464-wasm-shared-memory-object-v4.patch
> 
> Review of attachment 8930088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great!  R+ with nits and guard on the shared_memory pref in both
> DecodeMemoryLimits() and WasmJS.cpp's GetLimits().

Is that really where we want the check?  Consider a scenario where shared memory is allowed.  We compile a module and serialize it.  Later we disable the shared memory pref.  Later still we load the module from IDB and instantiate it, and that will work even if shared memory is now disabled.  That doesn't seem right.

So at a minimum we need something that checks during memory instantiation, probably in CreateWasmBuffer, though we could additionally have checks in the two spots you mention above.
I like those early checks b/c it means you can use WebAssembly.validate() and/or `new Memory({shared:true})` to feature-test, but you're totally right we need an instantiation check too.

Comment 34

a year ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38ba9478def1
Share memory mapping code between SharedArrayRawBuffer and WasmArrayRawBuffer. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8d3f0749e42
Preparatory cleanup. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/d704d1e5b453
Parse attributes for shared memory. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8da4918cba
Implement shared memory for WebAssembly. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/565ca96c4149
Test cases for wasm shared memory. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/576034b89b34
Wasm atomics gating on shared memory enabled.  rs=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/be8d2835531b
Test case for wasm atomics gating. rs=luke
You need to log in before you can comment on or make changes to this bug.