Baldr: Add current_memory and grow_memory

RESOLVED FIXED in Firefox 51

Status

()

Core
JavaScript Engine: JIT
--
enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: dbounov@mozilla.com, Assigned: dbounov@mozilla.com)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments, 15 obsolete attachments)

195.66 KB, patch
luke
: review+
Details | Diff | Splinter Review
17.62 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
Add current_memory/grow_memory. Requires:

1) Adding the 2 operators to the language
     - add to lexer/parser
     - add to AST/BinaryIterator
     - add to Baseline
     - add to WasmIonCompile
     - grow/current memory are both lowered into builtin calls, so they don't require any MIR changes
     - add to BinaryToText/BinaryToTextExperimental

2) Implementing the semantics of the operators
(Assignee)

Comment 1

2 years ago
Created attachment 8772630 [details] [diff] [review]
mem_lang_changes.diff

Patch doing 1) of the previous comment (except "add to BinaryToText/BinaryToTextExperimental").

The operators are currently no-ops, just incrementing/returning a dummy variable.
(Assignee)

Comment 2

2 years ago
Created attachment 8777630 [details] [diff] [review]
grow_mem_candidate_0.diff

First shot at the grow memory ops. The patch is a *lil* big. Involves the following:

1. Adding current_memory/grow_memory to the parser/ast.
    grow_memory is just an instance of a unary op
    current_memory is an instance of the newly added "nullary op". AstNop
    should probably turn into an AstNullaryOp as well
2. Extending ArrayBufferObject/SharedArrayBufferObject with mappedSize() and growForWasm()
    ArrayBufferObject now features an ArrayRawBufferObject (similarly to SharedArrayBufferObject) right before the data pointer. It holds the mapped size.

3. ASMJS_MAY_USE_SIGNAL_HANDLER* #defines are removed as all 3 architectures may use some flavour of signals. The decision whether signals are available is now dynamic, stored entirely in the SignalUsage and Assumptions structs.

4. The meaning of SignalUsage.forOOB is now different. Before it used to mean "we have signal hanndlers and are 64bit". Now its just "we have signal handlers". Correspondingly Assumptions got a new flags "has4GGuardRegions".

5. All the places that decide if we should need/emit bounds checks are updated given (4)

6. The sizes of bounds check (see SpecializeToMemory) is now different - if we have signals and are not on x64 then its the mapped size (which may go up to maxheaplenth) instead of just the current heap length.

7. ArrayBufferObject::createForWasm's logic for deciding how much to mmap is also changed, accounting for the case when we have signals, but no 4G regions - then it maps up to the max heap length

8. Added a new type of AsmJSCall callee - an instance method call. An instance method call passes the Instance* as an implicit 1st argument.

9. Used (8) to add entry points from wasm for current and grow memory

10. Added Instance::currentMemory and Instance::growMemory using the above machinery.

11. New tests (wasm/basic-grow-memory.js)

Whats missing:

1) Cleanup - merge AstNop into AstNullaryOp

2) If we fail to map the requested maximum memory size, try with a smaller size instead of failing outright
Attachment #8772630 - Attachment is obsolete: true
Attachment #8777630 - Flags: review?(sunfish)
Attachment #8777630 - Flags: review?(luke)
(Assignee)

Comment 3

2 years ago
I forgot - also missing adding a "max" field to the wasm JS memory objects.

Comment 4

2 years ago
Drive-by comments: passInstance() should be structured differently, since there will be no 64-bit pointers on 32-bit systems and vice versa.  The appropriate structure is

#if defined(JS_CODEGEN_X64)
void passInstance(...) {
  ...  // can use ScratchI64, assume pointer is 8 bytes
}
#elif defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_ARM)
void passInstance(...) {
  ...  // assume pointer is 4 bytes
}
#else
void passInstance(...) {
  MOZ_CRASH()
}
#endif

Also I don't understand why this code pushes and pops the scratch registers.
(Assignee)

Comment 5

2 years ago
Thanks for the feedback Lars!
(In reply to Lars T Hansen [:lth] from comment #4)
> Drive-by comments: passInstance() should be structured differently, since
> there will be no 64-bit pointers on 32-bit systems and vice versa.  The
> appropriate structure is
> 
Will fix the structure.

> #if defined(JS_CODEGEN_X64)
> void passInstance(...) {
>   ...  // can use ScratchI64, assume pointer is 8 bytes
> }
> #elif defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_ARM)
> void passInstance(...) {
>   ...  // assume pointer is 4 bytes
> }
> #else
> void passInstance(...) {
>   MOZ_CRASH()
> }
> #endif
> 
> Also I don't understand why this code pushes and pops the scratch registers.

Paranoia. I don't understand everything in baseline so I wasn't sure I properly allocated a scratch register (especially for x64 - ABINonArgReturnReg0). If its safe not to trample them, I will remove the push/pop
(Assignee)

Comment 6

2 years ago
(In reply to Lars T Hansen [:lth] from comment #4)
> Drive-by comments: passInstance() should be structured differently, since
> there will be no 64-bit pointers on 32-bit systems and vice versa.  The
> appropriate structure is
> 
> #if defined(JS_CODEGEN_X64)
> void passInstance(...) {
>   ...  // can use ScratchI64, assume pointer is 8 bytes

I don't see a ScratchI64 type.

> }
> #elif defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_ARM)
> void passInstance(...) {
>   ...  // assume pointer is 4 bytes
> }
> #else
> void passInstance(...) {
>   MOZ_CRASH()
> }
> #endif
> 
> Also I don't understand why this code pushes and pops the scratch registers.
(Assignee)

Comment 7

2 years ago
Created attachment 8777996 [details] [diff] [review]
grow_mem_candidate_1_lth_and_backoff.diff

1) Addressed comments from Lars

2) Added a fail-case in createForWasm that searches for the maximum allocatable memory between minMemory and the requested max. It does so using binary search.

3) Added a grow-memory test case in tests/wasm/spec/. If approved, should probably migrate upstream to ml-proto
Attachment #8777630 - Attachment is obsolete: true
Attachment #8777630 - Flags: review?(sunfish)
Attachment #8777630 - Flags: review?(luke)
Attachment #8777996 - Flags: review?(sunfish)
Attachment #8777996 - Flags: review?(luke)

Comment 8

2 years ago
(In reply to dbounov from comment #6)
> (In reply to Lars T Hansen [:lth] from comment #4)
> > Drive-by comments: passInstance() should be structured differently, since
> > there will be no 64-bit pointers on 32-bit systems and vice versa.  The
> > appropriate structure is
> > 
> > #if defined(JS_CODEGEN_X64)
> > void passInstance(...) {
> >   ...  // can use ScratchI64, assume pointer is 8 bytes
> 
> I don't see a ScratchI64 type.

Man, I just hate it when code does not simply materialize the instant I think of it.  There's all this tedious keyboarding to do first.

There needs to be one of those.  It's somewhat likely that Hannes has already invented it, because he'll really need it.
Comment on attachment 8777996 [details] [diff] [review]
grow_mem_candidate_1_lth_and_backoff.diff

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

Overall this looks good! I've made a first round of comments here, and there's a pretty significant patch sequence from Luke that just landed that will cause some merge conflicts here, so let's start with that and then do another round.

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +1982,5 @@
> +        } else  {
> +            MOZ_RELEASE_ASSERT(argLoc.kind() == ABIArg::Stack);
> +            // FIXME(dbounov): Is it always safe to use ABINonArgReturnReg0 as a scratch
> +            // register here? Can't find a ScratchI64.
> +            Register scratch = ABINonArgReturnReg0;

Since this is for use before the call, this can use ABINonArgReg0.

::: js/src/jit/MIR.h
@@ +13494,5 @@
>  {
>    public:
>      class Callee {
>        public:
> +        enum Which { Internal, Dynamic, Builtin, BuiltinMethod };

It'd be handy to have a quick comment about the difference between Builtin and BuiltinMethod here.

@@ +13608,5 @@
>      bool possiblyCalls() const override {
>          return true;
>      }
> +
> +    const ABIArg& instanceArg() {

This member function can be const.

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +1519,5 @@
> +        if (resArg.kind() == ABIArg::GPR) {
> +            masm.loadPtr(Address(WasmTlsReg, offsetof(wasm::TlsData, instance)), resArg.gpr());
> +        } else if (resArg.kind() == ABIArg::Stack) {
> +            // Safe to use ABINonArgReturnReg0 since its the last thing before the call
> +            Register scratch = ABINonArgReturnReg0;

As above, this should use AbiNonArgReg0.

::: js/src/vm/ArrayBufferObject-inl.h
@@ +50,5 @@
> +        return buf->as<ArrayBufferObject>().mappedSize();
> +    return buf->as<SharedArrayBufferObject>().mappedSize();
> +}
> +
> +inline uint32_t

It looks like the underlying growForWasm calls return bool, so it seems that AnyArrayGrowForWasm should return a bool too, especially so that it doesn't create confusion about whether it behaves like grow_memory's return value, which *is* a uint32_t.

::: js/src/vm/ArrayBufferObject.cpp
@@ +383,5 @@
> +    uint64_t actualNumBytes = numBytes + gc::SystemPageSize();
> +
> +    MOZ_ASSERT(numBytes % wasm::PageSize == 0);
> +    MOZ_RELEASE_ASSERT(actualNumBytes <= actualMappedSize &&
> +                       actualMappedSize <= std::numeric_limits<size_t>::max());

std::numeric_limits<size_t>::max() is also known as SIZE_MAX.

@@ +466,5 @@
> +            // On other platforms search up to 64K granularity
> +            size_t delta = wasm::PageSize;
> +#endif
> +            while (max - min > delta) {
> +                size_t mid = (max + min) / 2;

Could max + min possibly overflow here? If so, this would be safer as "max + (max - min) / 2"

@@ +469,5 @@
> +            while (max - min > delta) {
> +                size_t mid = (max + min) / 2;
> +
> +                data = ArrayRawBufferObject::AllocateWasmMappedMemory(numBytes,
> +                                                                      mid);

SM coding style allows lines up to 99 columns, so this can all go on one line.

@@ +657,5 @@
> +
> +    size_t curSize = byteLength();
> +    uint8_t *dataEnd = data + curSize;
> +    size_t deltaBytes = delta * wasm::PageSize;
> +    size_t newSize = curSize + deltaBytes;

As in the SharedArrayBufferObject case below, can either the multiply and/or add overflow here?

@@ +670,5 @@
> +    if (deltaBytes && mprotect(dataEnd, deltaBytes, PROT_READ | PROT_WRITE))
> +        return false;
> +#  if defined(MOZ_VALGRIND) && defined(VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE)
> +    VALGRIND_ENABLE_ADDR_ERROR_REPORTING_IN_RANGE((unsigned char*)dataEnd, deltaBytes);
> +#  endif

Valgrind code in general doesn't need to be guarded by !XP_WIN.

::: js/src/vm/ArrayBufferObject.h
@@ +352,5 @@
>      uint8_t* dataPointer() const;
>      SharedMem<uint8_t*> dataPointerShared() const;
>      uint32_t byteLength() const;
> +    uint64_t mappedSize() const;
> +    bool growForWasm(uint32_t delta);

These kinds of functions that return a bool "success" value are good candidates for MOZ_MUST_USE attributes (here and elsewhere in the patch).

::: js/src/vm/SharedArrayObject.cpp
@@ +338,5 @@
> +        return true;
> +
> +    size_t curSize = byteLength();
> +    uint8_t *dataEnd = data + curSize;
> +    size_t deltaBytes = delta * wasm::PageSize;

Can this multiply overflow, or is there code protecting growForWasm against this?

@@ +339,5 @@
> +
> +    size_t curSize = byteLength();
> +    uint8_t *dataEnd = data + curSize;
> +    size_t deltaBytes = delta * wasm::PageSize;
> +    size_t newSize = curSize + delta * wasm::PageSize;

delta * wasm::PageSize is deltaBytes :-).

Also, can this add overflow?

::: js/src/vm/SharedArrayObject.h
@@ +45,5 @@
>  {
>    private:
>      mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> refcount_;
>      uint32_t length;
> +    uint64_t mappedSize_;

Is there a reason for making this a uint64_t rather than a size_t? For something representing a raw buffer in memory, I'd expect a size_t, but I'd be happy with a comment explaining the choice of uint64_t too :-).
Attachment #8777996 - Flags: review?(sunfish)
(Assignee)

Comment 10

2 years ago
Created attachment 8778523 [details] [diff] [review]
grow_mem_candidate_v3_dan_and_rb_luke.diff

Rebased after Luke's Aug 05 landing and Dan's first comments. In reply to Dan's comments:

(In reply to Dan Gohman [:sunfish] from comment #9)
> Comment on attachment 8777996 [details] [diff] [review]
> grow_mem_candidate_1_lth_and_backoff.diff
> 
> Review of attachment 8777996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this looks good! I've made a first round of comments here, and
> there's a pretty significant patch sequence from Luke that just landed that
> will cause some merge conflicts here, so let's start with that and then do
> another round.
> 
> ::: js/src/asmjs/WasmBaselineCompile.cpp
> @@ +1982,5 @@
> > +        } else  {
> > +            MOZ_RELEASE_ASSERT(argLoc.kind() == ABIArg::Stack);
> > +            // FIXME(dbounov): Is it always safe to use ABINonArgReturnReg0 as a scratch
> > +            // register here? Can't find a ScratchI64.
> > +            Register scratch = ABINonArgReturnReg0;
> 
> Since this is for use before the call, this can use ABINonArgReg0.

Fixed

> 
> ::: js/src/jit/MIR.h
> @@ +13494,5 @@
> >  {
> >    public:
> >      class Callee {
> >        public:
> > +        enum Which { Internal, Dynamic, Builtin, BuiltinMethod };
> 
> It'd be handy to have a quick comment about the difference between Builtin
> and BuiltinMethod here.
>

Fixed (now in WasmTypes.h)
 
> @@ +13608,5 @@
> >      bool possiblyCalls() const override {
> >          return true;
> >      }
> > +
> > +    const ABIArg& instanceArg() {
> 
> This member function can be const.
> 

Fixed.

> ::: js/src/jit/shared/CodeGenerator-shared.cpp
> @@ +1519,5 @@
> > +        if (resArg.kind() == ABIArg::GPR) {
> > +            masm.loadPtr(Address(WasmTlsReg, offsetof(wasm::TlsData, instance)), resArg.gpr());
> > +        } else if (resArg.kind() == ABIArg::Stack) {
> > +            // Safe to use ABINonArgReturnReg0 since its the last thing before the call
> > +            Register scratch = ABINonArgReturnReg0;
> 
> As above, this should use AbiNonArgReg0.
> 

Fixed.

> ::: js/src/vm/ArrayBufferObject-inl.h
> @@ +50,5 @@
> > +        return buf->as<ArrayBufferObject>().mappedSize();
> > +    return buf->as<SharedArrayBufferObject>().mappedSize();
> > +}
> > +
> > +inline uint32_t
> 
> It looks like the underlying growForWasm calls return bool, so it seems that
> AnyArrayGrowForWasm should return a bool too, especially so that it doesn't
> create confusion about whether it behaves like grow_memory's return value,
> which *is* a uint32_t.
> 
Fixed.

> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +383,5 @@
> > +    uint64_t actualNumBytes = numBytes + gc::SystemPageSize();
> > +
> > +    MOZ_ASSERT(numBytes % wasm::PageSize == 0);
> > +    MOZ_RELEASE_ASSERT(actualNumBytes <= actualMappedSize &&
> > +                       actualMappedSize <= std::numeric_limits<size_t>::max());
> 
> std::numeric_limits<size_t>::max() is also known as SIZE_MAX.
> 
Fixed.

> @@ +466,5 @@
> > +            // On other platforms search up to 64K granularity
> > +            size_t delta = wasm::PageSize;
> > +#endif
> > +            while (max - min > delta) {
> > +                size_t mid = (max + min) / 2;
> 
> Could max + min possibly overflow here? If so, this would be safer as "max +
> (max - min) / 2"

Fixed.

> 
> @@ +469,5 @@
> > +            while (max - min > delta) {
> > +                size_t mid = (max + min) / 2;
> > +
> > +                data = ArrayRawBufferObject::AllocateWasmMappedMemory(numBytes,
> > +                                                                      mid);
> 
> SM coding style allows lines up to 99 columns, so this can all go on one
> line.
> 
Fixed.

> @@ +657,5 @@
> > +
> > +    size_t curSize = byteLength();
> > +    uint8_t *dataEnd = data + curSize;
> > +    size_t deltaBytes = delta * wasm::PageSize;
> > +    size_t newSize = curSize + deltaBytes;
> 
> As in the SharedArrayBufferObject case below, can either the multiply and/or
> add overflow here?
> 

Fixed.

> @@ +670,5 @@
> > +    if (deltaBytes && mprotect(dataEnd, deltaBytes, PROT_READ | PROT_WRITE))
> > +        return false;
> > +#  if defined(MOZ_VALGRIND) && defined(VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE)
> > +    VALGRIND_ENABLE_ADDR_ERROR_REPORTING_IN_RANGE((unsigned char*)dataEnd, deltaBytes);
> > +#  endif
> 
> Valgrind code in general doesn't need to be guarded by !XP_WIN.
> 

Fixed.

> ::: js/src/vm/ArrayBufferObject.h
> @@ +352,5 @@
> >      uint8_t* dataPointer() const;
> >      SharedMem<uint8_t*> dataPointerShared() const;
> >      uint32_t byteLength() const;
> > +    uint64_t mappedSize() const;
> > +    bool growForWasm(uint32_t delta);
> 
> These kinds of functions that return a bool "success" value are good
> candidates for MOZ_MUST_USE attributes (here and elsewhere in the patch).
> 

Fixed.

> ::: js/src/vm/SharedArrayObject.cpp
> @@ +338,5 @@
> > +        return true;
> > +
> > +    size_t curSize = byteLength();
> > +    uint8_t *dataEnd = data + curSize;
> > +    size_t deltaBytes = delta * wasm::PageSize;
> 
> Can this multiply overflow, or is there code protecting growForWasm against
> this?
> 

Protected by Instance::growMemory. However added a release assert out of paranoia:

+
+    // Sanity. This should be guaranteed by Instance::growMemory
+    MOZ_RELEASE_ASSERT(
+            ((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX &&
+            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);

> @@ +339,5 @@
> > +
> > +    size_t curSize = byteLength();
> > +    uint8_t *dataEnd = data + curSize;
> > +    size_t deltaBytes = delta * wasm::PageSize;
> > +    size_t newSize = curSize + delta * wasm::PageSize;
> 
> delta * wasm::PageSize is deltaBytes :-).
> 
> Also, can this add overflow?
> 

Fixed. Also added release assert.

> ::: js/src/vm/SharedArrayObject.h
> @@ +45,5 @@
> >  {
> >    private:
> >      mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> refcount_;
> >      uint32_t length;
> > +    uint64_t mappedSize_;
> 
> Is there a reason for making this a uint64_t rather than a size_t? For
> something representing a raw buffer in memory, I'd expect a size_t, but I'd
> be happy with a comment explaining the choice of uint64_t too :-).

Fixed.
Attachment #8777996 - Attachment is obsolete: true
Attachment #8777996 - Flags: review?(luke)
Attachment #8778523 - Flags: review?(sunfish)
Attachment #8778523 - Flags: review?(luke)

Comment 11

2 years ago
Comment on attachment 8778523 [details] [diff] [review]
grow_mem_candidate_v3_dan_and_rb_luke.diff

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

This is a great start and good work.  Here is a mix of some nits and also some higher-level changes that I got from a first coarse-grained sweep over the patch.

::: js/src/asmjs/WasmInstance.cpp
@@ +286,5 @@
> +uint32_t
> +Instance::growMemory(uint32_t delta)
> +{
> +    if (!memory_)
> +        return (uint32_t) -1;

It should actually be a validation error to have grow_memory without any imported/defined memory (b/c there is no default memory).  So that means you can MOZ_RELEASE_ASSERT(memory_).

::: js/src/asmjs/WasmInstance.h
@@ +56,5 @@
>      static int32_t callImport_i32(Instance*, int32_t, int32_t, uint64_t*);
>      static int32_t callImport_i64(Instance*, int32_t, int32_t, uint64_t*);
>      static int32_t callImport_f64(Instance*, int32_t, int32_t, uint64_t*);
> +    static uint32_t growMemory_WasmEntry(Instance* instance, uint32_t delta);
> +    static uint32_t currentMemory_WasmEntry(Instance* instance);

To avoid the overload-resolution ambiguity for growMemory and also follow the above naming convention, could you change the suffix from WasmEntry to the return type (_i32, iirc)?

@@ +124,5 @@
>                         size_t* code,
>                         size_t* data) const;
> +
> +    uint32_t growMemory(uint32_t delta);
> +    uint32_t currentMemory();

Since these are not called except via the private static funtions, could you move these up into the private section, right under 'callImport' (which is in the same situation)?

::: js/src/asmjs/WasmIonCompile.cpp
@@ +895,5 @@
> +        if (inDeadCode())
> +            return true;
> +
> +        // Should only pass an instance once.
> +        MOZ_ASSERT(((int8_t)args->instanceArg_.kind()) == -1);

How about MOZ_ASSERT(args->instanceArg_ == ABIArg(), "should only pass instance once")?

@@ +980,5 @@
> +
> +            // If there is no instanceArg_ instanceArg_.kind() == -1
> +            if (call->instanceArg_.kind() == ABIArg::Stack) {
> +                uint32_t newOffset = call->instanceArg_.offsetFromArgBase() + call->spIncrement_;
> +                call->instanceArg_ = ABIArg(newOffset);

I think you can "inline" newOffset into ABIArg() without loss of clarity.  Also, I think the stackBytes increment should go at the end of the 'if' since otherwise you might think there was an ordering dependency, which would be odd.

@@ +1100,5 @@
>          return true;
>      }
>  
> +    bool builtinInstanceMethodCall(SymbolicAddress builtin, const CallCompileState& call, ValType ret,
> +                     MDefinition** def)

nit: indent 'MDefinition' to same column as 'SymbolicAddress'.

::: js/src/asmjs/WasmModule.cpp
@@ +17,5 @@
>   */
>  
>  #include "asmjs/WasmModule.h"
>  
> +#include "jsutil.h"

super-nit: per https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.23include_ordering you need a \n after this #include.

@@ +536,5 @@
> +#ifdef JS_CODEGEN_X64
> +        size_t mappedSize = legalizeMappedSize(wasm::MaxMappedSize);
> +#else
> +        size_t mappedSize = legalizeMappedSize(metadata_->maxMemoryLength);
> +#endif

If we consider WASM_HUGE_MEMORY as a fundamental platform assumption, I think we could move the #ifdef to inside legalizeMappedSize.

@@ +546,4 @@
>          if (!buffer)
> +            // TODO(dbounov): What happens when we fail to allocate maxMemoryLength, but *can*
> +            //  allocate min memory? The code is already compiled with forOOB, so it expects
> +            //  the memory up to max to be allocated. Do we re-try with a back off?

With the binary search in createForWasm, can this comment now be removed?

::: js/src/asmjs/WasmTextToBinary.cpp
@@ +758,5 @@
>                  return WasmToken(WasmToken::CallImport, begin, cur_);
>              return WasmToken(WasmToken::Call, begin, cur_);
>          }
> +        if (consume(u"current_memory"))
> +                return WasmToken(WasmToken::NullaryOpcode, Expr::CurrentMemory, begin, cur_);

nit: un-indent 4 spaces

::: js/src/asmjs/WasmTypes.cpp
@@ +659,5 @@
> +    size_t res = requestedSize;
> +
> +#ifdef JS_CODEGEN_ARM
> +    // On Arm round so that it fits in a single instruction
> +    res = RoundUpToNextValidAsmJSHeapLength(res);

Although it's probably commutative *at the moment*, I think the RoundUpToNextValidAsmJSHeapLength() should go at the end so that it's syntactically clear that we always end with a valid ARM constant.

I also think that we should rename to "RoundUpToNextValidARMLengthImmediate()" and move it into WasmTypes.cpp (right above this function) since now it's part of wasm.

::: js/src/asmjs/WasmTypes.h
@@ +824,5 @@
>      { }
>  
>      void* patchMemoryPtrImmAt(uint8_t* code) const { return code + nextInsOffset_; }
> +    void offsetBy(uint32_t offset) { nextInsOffset_ += offset; insnOffset_ += offset; }
> +    uint32_t insnOffset() const { return insnOffset_; }

I don't think we need to keep track of this just for the precision of the MOZ_RELEASE_ASSERT: we'll have already checked that the access is in module's instance's memory and that the pc is inside wasm code.  There are tons of heap accesses so this saves MiBs and probably non-trivial deserialization time.

@@ +1031,5 @@
>  {
>      // NB: these fields are serialized as a POD in Assumptions.
>      bool forOOB;
>      bool forInterrupt;
> +    bool forUnalignedAccess;

forUnalignedAccess seems to be unused.

Also, I thought the more-recent plan was to simply make HaveSignalHandlers() a precondition for asm.js/wasm compilation.  Other than removing a whole mode to consider that isn't needed on any tier-1 platform, there is also the fact that I believe we end up *relying* on being able to "legalize" memory sizes (so that, e.g., the length always fits in an ARM Imm8) for correctness and adding support for the general case would mean even more complexity.  In this case, we could completely remove this struct.

Going further, I think we should simply remove the has4GGuardRegions and related bools and go back to a simplified #ifdef.  I know I advocated for the removal of all the #ifdefs earlier, but that was when we gated all of signal handling on a dynamic bool.  With the above change, though, I think we could get even simpler by having an #ifdef WASM_HUGE_MEMORY (which is simply defined if defined(JS_CODEGEN_X64) but later could get ARM64 and it'd also help us grep to see where we depend on this if we later wanted to allow non-huge mappings on 64-bit).  Most of the ASMJS_MAY_USE_SIGNAL_HANDLERS removal in this patch would stay because ASMJS_MAY_USE_SIGNAL_HANDLERS guarded signal-handling use at all and we're now requiring that always; I think only the needsBoundsCheck() and AB-allocation logic cares about WASM_HUGE_MEMORY.

This would leave us in the state:
 32-bit => we have signal handling and guard/slop pages
 64-bit => we have the huge mmaping and no bounds checks (except as required for atomics)

@@ +1298,3 @@
>  #endif
>  
> +size_t legalizeMappedSize(size_t requestedSize);

SM naming convention is Legalize*.  Also, we're sorta *returning* the mapped size, so I'd rename to LegalizeMemoryLength().

::: js/src/jit/MIR.cpp
@@ +5384,5 @@
> +MWasmCall*
> +MWasmCall::New(TempAllocator& alloc, const wasm::CallSiteDesc& desc, const wasm::CalleeDesc& callee,
> +               const ABIArg& instanceArg, const Args& args, MIRType resultType,
> +               uint32_t spIncrement, uint32_t tlsStackOffset,
> +               MDefinition* tableIndex) {

nit { on new line

@@ +5389,5 @@
> +    MWasmCall *call = MWasmCall::New(alloc, desc, callee, args, resultType, spIncrement,
> +                                     tlsStackOffset, tableIndex);
> +
> +    // instanceArg should be specified ONLY for CalleeDesc::BuiltinInstanceMethod
> +    MOZ_ASSERT(callee.which() == wasm::CalleeDesc::BuiltinInstanceMethod);

How about renaming to MWasmCall::NewBuiltinInstanceMethodCall() and then you don't need to take a CalleeDesc or tableIndex arguments.

@@ +5390,5 @@
> +                                     tlsStackOffset, tableIndex);
> +
> +    // instanceArg should be specified ONLY for CalleeDesc::BuiltinInstanceMethod
> +    MOZ_ASSERT(callee.which() == wasm::CalleeDesc::BuiltinInstanceMethod);
> +    MOZ_ASSERT(((int8_t)instanceArg.kind()) != -1);

Also, I think this assert makes more sense in FunctionCompiler::builtinInstanceMethodCall() since it's more of a local invariant of WasmIonCompile.cpp.

::: js/src/vm/ArrayBufferObject.cpp
@@ +642,5 @@
> +}
> +
> +bool
> +ArrayBufferObject::growForWasm(uint32_t delta)
> +{

Can you MOZ_ASSERT(isWasm())?

@@ +643,5 @@
> +
> +bool
> +ArrayBufferObject::growForWasm(uint32_t delta)
> +{
> +    uint8_t *data = dataPointer();

nit: SM style is now to put the * next to the type, not identifier.  Can you change this globally in the patch? (I see a few other cases.)

@@ +646,5 @@
> +{
> +    uint8_t *data = dataPointer();
> +
> +    if (!data)
> +        return false;

How can this case be hit?  (isWasm() array buffers aren't detachable.)

@@ +656,5 @@
> +
> +    // Sanity. This should be guaranteed by Instance::growMemory
> +    MOZ_RELEASE_ASSERT(
> +            ((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX &&
> +            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);

nit: separate conjuncts into separate MOZ_RELEASE_ASSERT statements

@@ +662,5 @@
> +    uint8_t *dataEnd = data + curSize;
> +    size_t deltaBytes = delta * wasm::PageSize;
> +    size_t newSize = curSize + deltaBytes;
> +
> +    MOZ_RELEASE_ASSERT(newSize <= UINT32_MAX && newSize <= mappedSize());

So, IIUC, if you don't specify a maximum, we'll use UINT32_MAX, that will clamped by Legalize, and then we'll end up reserving 1gb (for 32-bit).  Thus, I can see that this function only maps in new memory.

I think what needs to happen instead is:
 - we change maxMemorySize to a Maybe<uint32_t> so we can distinguish "no max specified" from "a max of UINT32_MAX specified"
 - the allocation is for Legalize(max ? max : initial)
 - if !max, then we don't do this mprotect, we do a realloc and have to repatch all the length checks.

I think probably a good idea to split out the realloc/repatch part into a follow-up patch and just have grow_memory fail if you don't specify a maximum in this patch.

@@ +678,5 @@
> +    VALGRIND_ENABLE_ADDR_ERROR_REPORTING_IN_RANGE((unsigned char*)dataEnd, deltaBytes);
> +#  endif
> +
> +    MemProfiler::SampleNative(dataEnd, deltaBytes);
> +    setByteLength(newSize);

So the semantics of ArrayBuffer lengths are that they are immutable, so we can't unfortunately just do this.  Instead JS.md says that a new ArrayBuffer is created which aliases the same memory:
  https://github.com/WebAssembly/design/blob/master/JS.md#webassemblymemoryprototypegrow
Getting this right involves some VM internals so maybe I can help out here.

::: js/src/vm/SharedArrayObject.cpp
@@ +326,5 @@
>          buf.byteLength() / buf.rawBufferObject()->refcount();
>  }
>  
> +bool
> +SharedArrayBufferObject::growForWasm(uint32_t delta)

I think it's currently impossible to call this function so I'd remove it entirely.  Also perhaps good to revert the other changes to this file.  I expect that, when we do wasm threads, we'll remove our asm.js+SharedArrayBuffer+atomics magic since there will be approx 0 usage at that point and it just adds complexity.  Instead, I think we'll refactor wasm::Memory to be the real owner of memory and SAB/AB will just alias it.
Attachment #8778523 - Flags: review?(luke)
(Assignee)

Comment 12

2 years ago
Created attachment 8779603 [details] [diff] [review]
grow_mem_candidate_v9.diff

I fixed most of the issues. Some comments/questions below and in inline responses (look for TODOs)

- I haven't removed has4GGuardRegions yet. We currently support disabling signals for testing. Have we decided we are not doing that?
If so I will remove has4GGuardRegions in this patch.

- Haven't removed the SignalUsage struct - that seems like a large sensitive change that should be kept separate, if we do it

- ArrayBuffer byte length field is immutable - waiting for help from Luke on that one.

Itemized reponse:

(In reply to Luke Wagner [:luke] from comment #11)
> Comment on attachment 8778523 [details] [diff] [review]
> grow_mem_candidate_v3_dan_and_rb_luke.diff
> 
> Review of attachment 8778523 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a great start and good work.  Here is a mix of some nits and also
> some higher-level changes that I got from a first coarse-grained sweep over
> the patch.

I like your definition of coarse-grained :)

> 
> ::: js/src/asmjs/WasmInstance.cpp
> @@ +286,5 @@
> > +uint32_t
> > +Instance::growMemory(uint32_t delta)
> > +{
> > +    if (!memory_)
> > +        return (uint32_t) -1;
> 
> It should actually be a validation error to have grow_memory without any
> imported/defined memory (b/c there is no default memory).  So that means you
> can MOZ_RELEASE_ASSERT(memory_).

Fixed.

> 
> ::: js/src/asmjs/WasmInstance.h
> @@ +56,5 @@
> >      static int32_t callImport_i32(Instance*, int32_t, int32_t, uint64_t*);
> >      static int32_t callImport_i64(Instance*, int32_t, int32_t, uint64_t*);
> >      static int32_t callImport_f64(Instance*, int32_t, int32_t, uint64_t*);
> > +    static uint32_t growMemory_WasmEntry(Instance* instance, uint32_t delta);
> > +    static uint32_t currentMemory_WasmEntry(Instance* instance);
> 
> To avoid the overload-resolution ambiguity for growMemory and also follow
> the above naming convention, could you change the suffix from WasmEntry to
> the return type (_i32, iirc)?

Fixed.

> 
> @@ +124,5 @@
> >                         size_t* code,
> >                         size_t* data) const;
> > +
> > +    uint32_t growMemory(uint32_t delta);
> > +    uint32_t currentMemory();
> 
> Since these are not called except via the private static funtions, could you
> move these up into the private section, right under 'callImport' (which is
> in the same situation)?

Fixed.

> 
> ::: js/src/asmjs/WasmIonCompile.cpp
> @@ +895,5 @@
> > +        if (inDeadCode())
> > +            return true;
> > +
> > +        // Should only pass an instance once.
> > +        MOZ_ASSERT(((int8_t)args->instanceArg_.kind()) == -1);
> 
> How about MOZ_ASSERT(args->instanceArg_ == ABIArg(), "should only pass
> instance once")?
> 

Fixed. This required adding the == and != operators to ABIArg.

> @@ +980,5 @@
> > +
> > +            // If there is no instanceArg_ instanceArg_.kind() == -1
> > +            if (call->instanceArg_.kind() == ABIArg::Stack) {
> > +                uint32_t newOffset = call->instanceArg_.offsetFromArgBase() + call->spIncrement_;
> > +                call->instanceArg_ = ABIArg(newOffset);
> 
> I think you can "inline" newOffset into ABIArg() without loss of clarity. 
> Also, I think the stackBytes increment should go at the end of the 'if'
> since otherwise you might think there was an ordering dependency, which
> would be odd.
> 

Fixed.

> @@ +1100,5 @@
> >          return true;
> >      }
> >  
> > +    bool builtinInstanceMethodCall(SymbolicAddress builtin, const CallCompileState& call, ValType ret,
> > +                     MDefinition** def)
> 
> nit: indent 'MDefinition' to same column as 'SymbolicAddress'.
> 

Fixed.

> ::: js/src/asmjs/WasmModule.cpp
> @@ +17,5 @@
> >   */
> >  
> >  #include "asmjs/WasmModule.h"
> >  
> > +#include "jsutil.h"
> 
> super-nit: per
> https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.
> 23include_ordering you need a \n after this #include.
> 

Fixed.

> @@ +536,5 @@
> > +#ifdef JS_CODEGEN_X64
> > +        size_t mappedSize = legalizeMappedSize(wasm::MaxMappedSize);
> > +#else
> > +        size_t mappedSize = legalizeMappedSize(metadata_->maxMemoryLength);
> > +#endif
> 
> If we consider WASM_HUGE_MEMORY as a fundamental platform assumption, I
> think we could move the #ifdef to inside legalizeMappedSize.
> 

Fixed. For now using JS_CODEGEN_X64 instead of WASM_HUGE_MEMORY inside LegalizeMaxMemorySize.

> @@ +546,4 @@
> >          if (!buffer)
> > +            // TODO(dbounov): What happens when we fail to allocate maxMemoryLength, but *can*
> > +            //  allocate min memory? The code is already compiled with forOOB, so it expects
> > +            //  the memory up to max to be allocated. Do we re-try with a back off?
> 
> With the binary search in createForWasm, can this comment now be removed?
> 

Fixed - Removed.

> ::: js/src/asmjs/WasmTextToBinary.cpp
> @@ +758,5 @@
> >                  return WasmToken(WasmToken::CallImport, begin, cur_);
> >              return WasmToken(WasmToken::Call, begin, cur_);
> >          }
> > +        if (consume(u"current_memory"))
> > +                return WasmToken(WasmToken::NullaryOpcode, Expr::CurrentMemory, begin, cur_);
> 
> nit: un-indent 4 spaces
> 

Fixed.

> ::: js/src/asmjs/WasmTypes.cpp
> @@ +659,5 @@
> > +    size_t res = requestedSize;
> > +
> > +#ifdef JS_CODEGEN_ARM
> > +    // On Arm round so that it fits in a single instruction
> > +    res = RoundUpToNextValidAsmJSHeapLength(res);
> 
> Although it's probably commutative *at the moment*, I think the
> RoundUpToNextValidAsmJSHeapLength() should go at the end so that it's
> syntactically clear that we always end with a valid ARM constant.
> 

Fixed.

> I also think that we should rename to
> "RoundUpToNextValidARMLengthImmediate()" and move it into WasmTypes.cpp
> (right above this function) since now it's part of wasm.
> 

Fixed. I preserved the original RoundUpToNextValidAsmJSHeapLength and IsValidAsmJSHeapLength, and
have them just call out to their wasm::.*ARM.* counterparts.

> ::: js/src/asmjs/WasmTypes.h
> @@ +824,5 @@
> >      { }
> >  
> >      void* patchMemoryPtrImmAt(uint8_t* code) const { return code + nextInsOffset_; }
> > +    void offsetBy(uint32_t offset) { nextInsOffset_ += offset; insnOffset_ += offset; }
> > +    uint32_t insnOffset() const { return insnOffset_; }
> 
> I don't think we need to keep track of this just for the precision of the
> MOZ_RELEASE_ASSERT: we'll have already checked that the access is in
> module's instance's memory and that the pc is inside wasm code.  There are
> tons of heap accesses so this saves MiBs and probably non-trivial
> deserialization time.
> 

Fixed.

> @@ +1031,5 @@
> >  {
> >      // NB: these fields are serialized as a POD in Assumptions.
> >      bool forOOB;
> >      bool forInterrupt;
> > +    bool forUnalignedAccess;
> 
> forUnalignedAccess seems to be unused.
> 

Fixed.

> Also, I thought the more-recent plan was to simply make HaveSignalHandlers()
> a precondition for asm.js/wasm compilation.  Other than removing a whole
> mode to consider that isn't needed on any tier-1 platform, there is also the
> fact that I believe we end up *relying* on being able to "legalize" memory
> sizes (so that, e.g., the length always fits in an ARM Imm8) for correctness
> and adding support for the general case would mean even more complexity.  In
> this case, we could completely remove this struct.
> 
> Going further, I think we should simply remove the has4GGuardRegions and
> related bools and go back to a simplified #ifdef.  I know I advocated for
> the removal of all the #ifdefs earlier, but that was when we gated all of
> signal handling on a dynamic bool.  With the above change, though, I think
> we could get even simpler by having an #ifdef WASM_HUGE_MEMORY (which is
> simply defined if defined(JS_CODEGEN_X64) but later could get ARM64 and it'd
> also help us grep to see where we depend on this if we later wanted to allow
> non-huge mappings on 64-bit).  Most of the ASMJS_MAY_USE_SIGNAL_HANDLERS
> removal in this patch would stay because ASMJS_MAY_USE_SIGNAL_HANDLERS
> guarded signal-handling use at all and we're now requiring that always; I
> think only the needsBoundsCheck() and AB-allocation logic cares about
> WASM_HUGE_MEMORY.
> 
> This would leave us in the state:
>  32-bit => we have signal handling and guard/slop pages
>  64-bit => we have the huge mmaping and no bounds checks (except as required
> for atomics)
> 

TODO: Waiting on comments before removing has4GGuardRegions

> @@ +1298,3 @@
> >  #endif
> >  
> > +size_t legalizeMappedSize(size_t requestedSize);
> 
> SM naming convention is Legalize*.  Also, we're sorta *returning* the mapped
> size, so I'd rename to LegalizeMemoryLength().
> 

Fixed. Renamed to LegalizeMaxMemoryLength

> ::: js/src/jit/MIR.cpp
> @@ +5384,5 @@
> > +MWasmCall*
> > +MWasmCall::New(TempAllocator& alloc, const wasm::CallSiteDesc& desc, const wasm::CalleeDesc& callee,
> > +               const ABIArg& instanceArg, const Args& args, MIRType resultType,
> > +               uint32_t spIncrement, uint32_t tlsStackOffset,
> > +               MDefinition* tableIndex) {
> 
> nit { on new line
> 

Fixed.

> @@ +5389,5 @@
> > +    MWasmCall *call = MWasmCall::New(alloc, desc, callee, args, resultType, spIncrement,
> > +                                     tlsStackOffset, tableIndex);
> > +
> > +    // instanceArg should be specified ONLY for CalleeDesc::BuiltinInstanceMethod
> > +    MOZ_ASSERT(callee.which() == wasm::CalleeDesc::BuiltinInstanceMethod);
> 
> How about renaming to MWasmCall::NewBuiltinInstanceMethodCall() and then you
> don't need to take a CalleeDesc or tableIndex arguments.

Fixed. Note that it doesn't save us the CalleeDesc, as we still need the symbolicaddress. It does save us
the tableIndex argument, and the naming is clearer.

> 
> @@ +5390,5 @@
> > +                                     tlsStackOffset, tableIndex);
> > +
> > +    // instanceArg should be specified ONLY for CalleeDesc::BuiltinInstanceMethod
> > +    MOZ_ASSERT(callee.which() == wasm::CalleeDesc::BuiltinInstanceMethod);
> > +    MOZ_ASSERT(((int8_t)instanceArg.kind()) != -1);
> 
> Also, I think this assert makes more sense in
> FunctionCompiler::builtinInstanceMethodCall() since it's more of a local
> invariant of WasmIonCompile.cpp.
> 
 
Not changed. - I'm not sure the assertion is local to WasmionCompile. Codegen relies on the instanceArg member being initialized. Thats why it seems cleaner to check this at the MIR instantiation boundary.

> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +642,5 @@
> > +}
> > +
> > +bool
> > +ArrayBufferObject::growForWasm(uint32_t delta)
> > +{
> 
> Can you MOZ_ASSERT(isWasm())?
> 

Fixed.

> @@ +643,5 @@
> > +
> > +bool
> > +ArrayBufferObject::growForWasm(uint32_t delta)
> > +{
> > +    uint8_t *data = dataPointer();
> 
> nit: SM style is now to put the * next to the type, not identifier.  Can you
> change this globally in the patch? (I see a few other cases.)
> 

Fixed.

> @@ +646,5 @@
> > +{
> > +    uint8_t *data = dataPointer();
> > +
> > +    if (!data)
> > +        return false;
> 
> How can this case be hit?  (isWasm() array buffers aren't detachable.)

Fixed - changed to a MOZ_RELEASE_ASSERT.

> 
> @@ +656,5 @@
> > +
> > +    // Sanity. This should be guaranteed by Instance::growMemory
> > +    MOZ_RELEASE_ASSERT(
> > +            ((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX &&
> > +            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);
> 
> nit: separate conjuncts into separate MOZ_RELEASE_ASSERT statements
> 

Fixed.

> @@ +662,5 @@
> > +    uint8_t *dataEnd = data + curSize;
> > +    size_t deltaBytes = delta * wasm::PageSize;
> > +    size_t newSize = curSize + deltaBytes;
> > +
> > +    MOZ_RELEASE_ASSERT(newSize <= UINT32_MAX && newSize <= mappedSize());
> 
> So, IIUC, if you don't specify a maximum, we'll use UINT32_MAX, that will
> clamped by Legalize, and then we'll end up reserving 1gb (for 32-bit). 
> Thus, I can see that this function only maps in new memory.
> 
> I think what needs to happen instead is:
>  - we change maxMemorySize to a Maybe<uint32_t> so we can distinguish "no
> max specified" from "a max of UINT32_MAX specified"
>  - the allocation is for Legalize(max ? max : initial)
>  - if !max, then we don't do this mprotect, we do a realloc and have to
> repatch all the length checks.
> 
> I think probably a good idea to split out the realloc/repatch part into a
> follow-up patch and just have grow_memory fail if you don't specify a
> maximum in this patch.
> 

Fixed. See comments on tests added in basic-grow-memory.js
Leaving the realloc/repatch stuff for future pushes.

> @@ +678,5 @@
> > +    VALGRIND_ENABLE_ADDR_ERROR_REPORTING_IN_RANGE((unsigned char*)dataEnd, deltaBytes);
> > +#  endif
> > +
> > +    MemProfiler::SampleNative(dataEnd, deltaBytes);
> > +    setByteLength(newSize);
> 
> So the semantics of ArrayBuffer lengths are that they are immutable, so we
> can't unfortunately just do this.  Instead JS.md says that a new ArrayBuffer
> is created which aliases the same memory:
>  
> https://github.com/WebAssembly/design/blob/master/JS.
> md#webassemblymemoryprototypegrow
> Getting this right involves some VM internals so maybe I can help out here.
> 

TODO - need help from Luke

> ::: js/src/vm/SharedArrayObject.cpp
> @@ +326,5 @@
> >          buf.byteLength() / buf.rawBufferObject()->refcount();
> >  }
> >  
> > +bool
> > +SharedArrayBufferObject::growForWasm(uint32_t delta)
> 
> I think it's currently impossible to call this function so I'd remove it
> entirely.  Also perhaps good to revert the other changes to this file.  I
> expect that, when we do wasm threads, we'll remove our
> asm.js+SharedArrayBuffer+atomics magic since there will be approx 0 usage at
> that point and it just adds complexity.  Instead, I think we'll refactor
> wasm::Memory to be the real owner of memory and SAB/AB will just alias it.

Fixed. I can't completely remove growMemory/mappedSize from the shared array buffer due to inheritance from ArrayBufferObjectMaybeShared.
I made growMemory() a MOZ_CRASH.
I kept mappedSize() as is, to keep SharedArrayRawBufferObject and ArrayRawBufferObject symmetric.
Attachment #8778523 - Attachment is obsolete: true
Attachment #8778523 - Flags: review?(sunfish)
Attachment #8779603 - Flags: review?(sunfish)
Attachment #8779603 - Flags: review?(luke)

Comment 13

2 years ago
(In reply to dbounov from comment #12)
> - Haven't removed the SignalUsage struct - that seems like a large sensitive
> change that should be kept separate, if we do it

The reason I suggest removing SignalUsage is that I don't think this patch (or the current state of trunk, for that matter) is sound when we can have un-legalized memory lengths (remember the MOZ_ASSERT in Imm8?) and we can't legalize if !usesSignal.forOOB.  I was hoping that this patch, with the legalization and required-signals would take us from the current "you can crash ARM with a bad Memory length" state to a safe one.

> - I haven't removed has4GGuardRegions yet. We currently support disabling
> signals for testing. Have we decided we are not doing that?
> If so I will remove has4GGuardRegions in this patch.

As the patch currently defines it, has4GGuardRegions is unconditionally true for x64 and one must test (usesSignal.forOOB && has4GGuardRegions) everywhere.  This seems hazardous (the name is technically a lie and it's easy to forget the forOOB check) so I think has4GGuardRegions should imply usesSignal.forOOB.  But if we do that, then I think having a simple #ifdef WASM_HUGE_GUARD is simpler; this isn't just a rename of the old ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB; it's much more targeted and should only need a handful of uses.

Comment 14

2 years ago
Comment on attachment 8779603 [details] [diff] [review]
grow_mem_candidate_v9.diff

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

A few more comments:

::: js/src/asmjs/WasmTypes.cpp
@@ +22,5 @@
>  
>  #include "jslibmath.h"
>  #include "jsmath.h"
>  
> +#include "asmjs/AsmJS.h"

I think this #include shouldn't be necessary.

@@ +647,5 @@
>  {
>      return buildId.sizeOfExcludingThis(mallocSizeOf);
>  }
> +
> +static const size_t MinHeapLength = PageSize;

Since MinHeapLength only has meaning within asm.js, I'd keep this and the checks on this in AsmJS.cpp and MOZ_ASSERT(length % PageSize == 0) in all the functions below.

@@ +655,5 @@
> +//  or
> +//    2^24 * n for n >= 1.
> +//
> +//  These are the values that allow the heap length to be placed in a ARM Imm8 (8 bit base + 4 bit
> +//  scale).

Technically there are a bunch more valid ARM immediate lengths; could you perhaps say "We approximate the set of valid ARM immediates with the predicate:".  Later we may want to tighten this up to save memory.  Also, this comment is out-of-date, 12 should be 16.

::: js/src/asmjs/WasmTypes.h
@@ +1263,5 @@
> +
> +#ifdef JS_CODEGEN_X64
> +static const size_t MaxMappedSize = 2 * Uint32Range + PageSize;
> +#else
> +static const size_t MaxMappedSize = (1 << 30); // 1 Gig

I think this is an impl detail of LegalizeMaxMemoryLength() and only meaningful in the !defined(JS_CODEGEN_X64) case; could you move it down into the body of LegalizeMaxMemoryLength into the #else condition?

@@ +1268,5 @@
>  #endif
>  
> +bool IsValidARMLengthImmediate(uint32_t length);
> +uint32_t RoundUpToNextValidARMLengthImmediate(uint32_t length);
> +size_t LegalizeMaxMemoryLength(size_t requestedSize);

Sorry to nit on the name again, but I think it's not quite right: we're not legalizing a value which represents the maximum memory length; we're taking an exact memory length and asking "how much should I round it up?".  So again I'd request LegalizeMemoryLength().

::: js/src/vm/ArrayBufferObject.cpp
@@ +443,5 @@
>  
>      if (signalsForOOB) {
> +        // First try to map the maximum requested memory
> +        void* data = ArrayRawBufferObject::AllocateWasmMappedMemory(numBytes,
> +                                                                    wasm::LegalizeMaxMemoryLength(mapSize));

Shouldn't need to legalize again; but a good idea to MOZ_ASSERT(LegalizeMemoryLength(mapSize) == mapSize) at the start of the function.
(Assignee)

Comment 15

2 years ago
(In reply to Luke Wagner [:luke] from comment #13)
> (In reply to dbounov from comment #12)
> > - Haven't removed the SignalUsage struct - that seems like a large sensitive
> > change that should be kept separate, if we do it
> 
> The reason I suggest removing SignalUsage is that I don't think this patch
> (or the current state of trunk, for that matter) is sound when we can have
> un-legalized memory lengths (remember the MOZ_ASSERT in Imm8?) and we can't
> legalize if !usesSignal.forOOB.  I was hoping that this patch, with the

Technically speaking, we could make it safe by requiring users on ARM to use only valid min/max memory lengths no?

But I see your point - if we don't do that then on ARM at least, we shouldn't be able to turn signals off.

> legalization and required-signals would take us from the current "you can
> crash ARM with a bad Memory length" state to a safe one.
> 
> > - I haven't removed has4GGuardRegions yet. We currently support disabling
> > signals for testing. Have we decided we are not doing that?
> > If so I will remove has4GGuardRegions in this patch.
> 
> As the patch currently defines it, has4GGuardRegions is unconditionally true
> for x64 and one must test (usesSignal.forOOB && has4GGuardRegions)
> everywhere.  This seems hazardous (the name is technically a lie and it's
> easy to forget the forOOB check) so I think has4GGuardRegions should imply
> usesSignal.forOOB.  But if we do that, then I think having a simple #ifdef
> WASM_HUGE_GUARD is simpler; this isn't just a rename of the old
> ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB; it's much more targeted and should
> only need a handful of uses.
(Assignee)

Comment 16

2 years ago
(In reply to dbounov from comment #15)
> (In reply to Luke Wagner [:luke] from comment #13)
> > (In reply to dbounov from comment #12)
> > > - Haven't removed the SignalUsage struct - that seems like a large sensitive
> > > change that should be kept separate, if we do it
> > 
> > The reason I suggest removing SignalUsage is that I don't think this patch
> > (or the current state of trunk, for that matter) is sound when we can have
> > un-legalized memory lengths (remember the MOZ_ASSERT in Imm8?) and we can't
> > legalize if !usesSignal.forOOB.  I was hoping that this patch, with the
> 
> Technically speaking, we could make it safe by requiring users on ARM to use
> only valid min/max memory lengths no?
> 
> But I see your point - if we don't do that then on ARM at least, we
> shouldn't be able to turn signals off.
> 

Is there any case where we wouldn't be using signals forInterrupt?

> > legalization and required-signals would take us from the current "you can
> > crash ARM with a bad Memory length" state to a safe one.
> > 
> > > - I haven't removed has4GGuardRegions yet. We currently support disabling
> > > signals for testing. Have we decided we are not doing that?
> > > If so I will remove has4GGuardRegions in this patch.
> > 
> > As the patch currently defines it, has4GGuardRegions is unconditionally true
> > for x64 and one must test (usesSignal.forOOB && has4GGuardRegions)
> > everywhere.  This seems hazardous (the name is technically a lie and it's
> > easy to forget the forOOB check) so I think has4GGuardRegions should imply
> > usesSignal.forOOB.  But if we do that, then I think having a simple #ifdef
> > WASM_HUGE_GUARD is simpler; this isn't just a rename of the old
> > ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB; it's much more targeted and should
> > only need a handful of uses.

Comment 17

2 years ago
(In reply to dbounov from comment #15)
> Technically speaking, we could make it safe by requiring users on ARM to use
> only valid min/max memory lengths no?
> 
> But I see your point - if we don't do that then on ARM at least, we
> shouldn't be able to turn signals off.

Yeah, we could constrain these testing modes but if we're changing wasm semantics that means signals-off is not a valid shipping configuration and then what's the point.

> Is there any case where we wouldn't be using signals forInterrupt?

If we simply require wasm::HaveSignalHandlers() as a precondition to asm.js/wasm, then SignalUsage::forInterrupt will always be true and thus it could be removed too.
(Assignee)

Comment 18

2 years ago
Created attachment 8780364 [details] [diff] [review]
grow_mem_candidate_v13.diff

Patch Lucky 13 :)

-Removed SignalUsage and has4GGuardRegion.
-Added WASM_HUGE_MEMORY #define as the guard for using 4G Guard regions
-Added wasm::HaveSignalHandlers() as a precondition for Wasm/AsmJS in -HasCompilerSupport()
-Cleanup a lot of dead code
-Substitute suppressSignalHandlers with a ion.interrupt-without-signals JitOption (used in ion/iloop-no-signals.js)
-Made maxMemoryLength in metadata/module generator a Maybe<uint32_t>. We track this to distinguish "no max specified" and "max=uint32" for the benefit of choosing the right mappedSize. This is important especially on x86 when we have multiple instances and don't want to exhaust address space unless users explicitly WANT a lot of addr. space
Attachment #8779603 - Attachment is obsolete: true
Attachment #8779603 - Flags: review?(sunfish)
Attachment #8779603 - Flags: review?(luke)
Attachment #8780364 - Flags: review?(sunfish)
Attachment #8780364 - Flags: review?(luke)

Comment 19

2 years ago
Comment on attachment 8780364 [details] [diff] [review]
grow_mem_candidate_v13.diff

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

Great, this is *almost* ready to land.  Almost all nits, but I think I found one bug, as well as that one test that it'd be good to reenable, that'd I'd like to understand and see addressed in a new patch.

::: js/src/asmjs/AsmJS.cpp
@@ +8811,3 @@
>  static const size_t MinHeapLength = PageSize;
> +// The asm.js valid heap lengths are precisely the WASM valid heap lengths for ARM
> +// greater or equal to MinHeapLength

nit: could you put MinHeapLength under the comment then with a \n after MinHeapLength?

::: js/src/asmjs/WasmAST.h
@@ +833,5 @@
>          return globals_;
>      }
>  };
>  
> +// TODO(dbounov) Should I merge AstNop into this?

Sure (or, if not, remove the TODO before landing).

::: js/src/asmjs/WasmCode.cpp
@@ +114,5 @@
> +#else
> +    // On x64 with 4G guard regions we still might get some BCs due to atomics.
> +    // For those check up to the maxMemoryLength. This should be inside the 4G
> +    // guard region.
> +    uint32_t bc_limit = metadata.maxMemoryLength.valueOr(metadata.minMemoryLength);

nit: we don't usually name with _ in SM; how about just 'length'?

@@ +610,5 @@
>  
>      return &metadata_->codeRanges[match];
>  }
>  
> +#ifdef JS_CODEGEN_X64

WASM_HUGE_MEMORY (basically, here and every other place in the patch that says JS_CODEGEN_X64)

::: js/src/asmjs/WasmCode.h
@@ +416,5 @@
>    public:
>      ModuleKind            kind;
>      MemoryUsage           memoryUsage;
>      uint32_t              minMemoryLength;
> +    mozilla::Maybe<uint32_t> maxMemoryLength;

nit: b/c of the 'using mozilla::Maybe' in WasmTypes.h, you shouldn't need the mozilla:: here.

::: js/src/asmjs/WasmCompile.cpp
@@ +1600,5 @@
>  SharedModule
>  wasm::Compile(const ShareableBytes& bytecode, CompileArgs&& args, UniqueChars* error)
>  {
>      bool newFormat = args.assumptions.newFormat;
> +    MOZ_RELEASE_ASSERT(wasm::SignalHandlersAlreadyInstalled());

As a precondition, can you put it first with a \n after?

::: js/src/asmjs/WasmGenerator.h
@@ +53,3 @@
>      MemoryUsage               memoryUsage;
>      mozilla::Atomic<uint32_t> minMemoryLength;
> +    mozilla::Maybe<uint32_t>  maxMemoryLength;

nit: shouldn't need the mozilla:: here

::: js/src/asmjs/WasmInstance.cpp
@@ +285,5 @@
> +
> +uint32_t
> +Instance::growMemory(uint32_t delta)
> +{
> +    MOZ_RELEASE_ASSERT(memory_);

nit: \n after

@@ +287,5 @@
> +Instance::growMemory(uint32_t delta)
> +{
> +    MOZ_RELEASE_ASSERT(memory_);
> +    // Using uint64_t to avoid worrying about overflows in safety comp.
> +    uint64_t curNPages = currentMemory();

nit: we generally spell out "num" instead of "n" in names

@@ +292,5 @@
> +    uint64_t newNPages = curNPages + (uint64_t) delta;
> +    uint64_t maxMemoryByteLength =
> +        metadata().maxMemoryLength.valueOr(metadata().minMemoryLength);
> +
> +    if (newNPages * wasm::PageSize > maxMemoryByteLength)

Can we express this logic instead as:
  if (metadata().maxMemoryLength) {
     if (beyond mappedSize)
        return -1;
     if (!memory_->buffer().growForWasm(delta))
        return -1;
  } else {
     return -1;  // TODO: implement grow_memory without max
  }
?  That way we clear out where we'll do something totally different for the !max case.

::: js/src/asmjs/WasmIonCompile.cpp
@@ +886,5 @@
> +
> +        // Should only pass an instance once.
> +        MOZ_ASSERT(args->instanceArg_ == ABIArg());
> +        ValType ptrType = (sizeof(void*) == 4 ? ValType::I32 : ValType::I64);
> +        args->instanceArg_ = args->abi_.next(ToMIRType(ptrType));

You can replace this with MIRType::Pointer.

@@ +1099,5 @@
> +        CallSiteDesc desc(call.lineOrBytecode_, CallSiteDesc::Register);
> +        auto callee = CalleeDesc::builtinInstanceMethod(builtin);
> +        auto* ins = MWasmCall::NewBuiltinInstanceMethodCall(alloc(), desc, callee,
> +                call.instanceArg_, call.regArgs_, ToMIRType(ret), call.spIncrement_,
> +                MWasmCall::DontSaveTls);

nit: SM indentation style lines up args with opening ( of call.

::: js/src/asmjs/WasmModule.cpp
@@ +17,5 @@
>   */
>  
>  #include "asmjs/WasmModule.h"
>  
> +#include "jsutil.h"

#include still necessary?

@@ +524,5 @@
> +        // Its not an error to import a memory, whose mapped size is less than
> +        // the maxMemoryLength required for the module. This is the same as trying to
> +        // map up to maxMemoryLength but actually getting less.
> +        if (length < metadata_->minMemoryLength ||
> +            length > metadata_->maxMemoryLength.valueOr(UINT32_MAX)) {

Can you hoist a
  uint32_t maxMemoryLength = metadata_->maxMemoryLength.valueOr(UINT32_MAX);
above the if (with \n before 'buffer') so that you can reuse here and below in LegalizeMemoryLength(), keeping both expressions on a single line?

::: js/src/asmjs/WasmSignalHandlers.cpp
@@ +801,5 @@
>          return pc == instance->codeSegment().interruptCode() &&
>                 instance->codeSegment().containsFunctionPC(activation->resumePC()) &&
>                 instance->code().lookupMemoryAccess(activation->resumePC());
> +#else
> +        // TODO(dbounov): Is this safe??

Yes, I think so: the first two conditions are sufficient to identify this peculiar situation.  In fact, I'd remove the #ifdefs and lookupMemoryAccess test entirely (just so platforms behave the same).

@@ +818,5 @@
>      *ppc = EmulateHeapAccess(context, pc, faultingAddress, memoryAccess, *instance);
> +#elif defined(JS_CODEGEN_X86)
> +    *ppc = instance->codeSegment().outOfBoundsCode();
> +#else // ARM
> +    MOZ_CRASH("Unsupported architecture for windows.");

I don't think we have to worry about unaligned vs. out-of-bounds on-ARM-on-Windows; can just have all !WASM_HUGE_MEMORY redirect pc to outOfBoundsCode?

@@ +954,5 @@
>      *ppc = EmulateHeapAccess(&context, pc, faultingAddress, memoryAccess, *instance);
> +#elif defined(JS_CODEGEN_X86)
> +    *ppc = instance->codeSegment().outOfBoundsCode();
> +#else // ARM
> +    MOZ_CRASH("Unsupported architecture for MacOS X.");

ditto about having having all !WASM_HUGE_MEMORY redirect to outOfBoundsCode.

@@ +1181,5 @@
> +#elif defined(JS_CODEGEN_X86)
> +    *ppc = instance->codeSegment().outOfBoundsCode();
> +#elif defined(JS_CODEGEN_ARM)
> +    if (signal == Signal::BusError)
> +        *ppc = instance->codeSegment().unalignedAccessCode();

nit: SM style says to put { } around this since the else has {}s.  Or, you could
  MOZ_RELEASE_ASSERT(signal == Signal::BusError || signal == Signal::SegFault);
and neither branch needs {} :)

@@ +1187,5 @@
> +        MOZ_RELEASE_ASSERT(signal == Signal::SegFault);
> +        *ppc = instance->codeSegment().outOfBoundsCode();
> +    }
> +#else
> +    MOZ_CRASH("Unsupported architecture for Linux.");

ditto about having having all !WASM_HUGE_MEMORY redirect to outOfBoundsCode.

@@ +1412,4 @@
>  }
>  
> +bool
> +wasm::SignalHandlersAlreadyInstalled()

I think we don't need a new function here: wasm::HaveSignalHandlers() is only called after JSRuntime::init() which calls EnsureSignalHandlers() so we can:
 - pull the statics sTried and sResult out of ProcessHasSignalHandlers() so that wasm::SignalHandlers() can `MOZ_ASSERT(sTried); return sResult;`.
 - probably good to rename to sTriedToInstallSignalHandlers/sInstalledSignalHandlers

::: js/src/asmjs/WasmTypes.cpp
@@ +636,5 @@
> +        length = mozilla::RoundUpPow2(length);
> +    else
> +        length = (length + 0x00ffffff) & ~0x00ffffff;
> +
> +    MOZ_ASSERT(length % PageSize == 0);

Or perhaps MOZ_ASSERT(IsvalidARMLengthImmedaite(length))?

::: js/src/asmjs/WasmTypes.h
@@ +1236,5 @@
>  };
>  
> +#ifdef JS_CODEGEN_X64
> +#define WASM_HUGE_MEMORY
> +#endif

Perhaps move this down and put the Uint32Range/MappedRange consts inside this #ifdef to show the association?

::: js/src/builtin/TestingFunctions.cpp
@@ +517,5 @@
>  static bool
>  WasmUsesSignalForOOB(JSContext* cx, unsigned argc, Value* vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    args.rval().setBoolean(true);

Could you remove this function and dependent tests too?

::: js/src/jit-test/tests/asm.js/testHeapAccess.js
@@ +27,5 @@
>  
> +// TODO(dbounov): Root-cause why this fails
> +// TODO(dbounov): Is this test still relevant?
> +// Bug 1088655
> +//assertEq(asmLink(asmCompile('stdlib', 'foreign', 'heap', USE_ASM + 'var i32=new stdlib.Int32Array(heap); function f(i) {i=i|0;var j=0x10000;return (i32[j>>2] = i)|0 } return f'), this, null, buf)(1), 1);

It does seem like this should pass; can you enable this test?

::: js/src/jit-test/tests/wasm/basic-grow-memory.js
@@ +90,5 @@
> +assertEq(linearModule(1,2,[["GM", 0, 1], [op, "i32", "", "0", "65532"]])(), 1)
> +assertOOB(()=>linearModule(1,2,[["GM", 0, 1], [op, "i32", "", "0", "65536"]])())
> +
> +// Test that if we don't specify a maximum memory size, we can't grow.
> +// Note that this should change on both x64 and x86 when single threaded.

We need growing to work on all archs, not just x86/x64.

::: js/src/jit/MIR.cpp
@@ +5389,5 @@
> +    MWasmCall* call = MWasmCall::New(alloc, desc, callee, args, resultType, spIncrement,
> +                                     tlsStackOffset, nullptr);
> +
> +    // instanceArg should be specified ONLY for CalleeDesc::BuiltinInstanceMethod
> +    MOZ_ASSERT(callee.which() == wasm::CalleeDesc::BuiltinInstanceMethod);

How about having NewBuiltinInstanceMethodCall take a SymbolicAddress instead?

::: js/src/jit/MIR.h
@@ +13555,5 @@
> +    static MWasmCall* NewBuiltinInstanceMethodCall(TempAllocator& alloc,
> +                          const wasm::CallSiteDesc& desc,
> +                          const wasm::CalleeDesc& callee, const ABIArg& instanceArg,
> +                          const Args& args, MIRType resultType,
> +                          uint32_t spIncrement, uint32_t tlsStackOffset);

You can remove the tlsStackOffset argument (it's for wasm->wasm calls) and pass DontSaveTls internally.

::: js/src/jit/MIRGraph.cpp
@@ +113,5 @@
>      // We use signal-handlers on x64, but on x86 there isn't enough address
>      // space for a guard region.  Also, on x64 the atomic loads and stores
>      // can't (yet) use the signal handlers.
> +#ifdef WASM_HUGE_MEMORY
> +    if (!access->isAtomicAccess())

In some later patch, it'd be nice to remove these methods from MIRGenerator (now that they don't depend on MIRGenerator state) and make them methods of MWasmMemoryAccess.

::: js/src/jit/RegisterSets.h
@@ +1272,5 @@
>      bool argInRegister() const { return kind() != Stack; }
>      AnyRegister reg() const { return kind_ == GPR ? AnyRegister(gpr()) : AnyRegister(fpu()); }
> +
> +    bool operator==(const ABIArg& rhs) const {
> +        if (kind_ != rhs.kind_) return false;

nit: return on new line

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +1516,5 @@
>      const wasm::CallSiteDesc& desc = mir->desc();
>      const wasm::CalleeDesc& callee = mir->callee();
> +
> +    // If this is a builtin instance method call, we must have
> +    // reserved the first argument for passing in Instance*. Pass it in now.

Can you factor out this code that is common between Ion and Baseline into a wasmBuiltinMethodCall(SymbolicAddress) method in MacroAssembler.(h,cpp) similar to wasmCallImport/Indirect?

::: js/src/vm/ArrayBufferObject.cpp
@@ +390,4 @@
>      if (!data)
>          return nullptr;
>  
> +    // TODO(dbounov): Note we will waste a page on zero-sized memories here

Comment makes sense to keep; can you remove the TODO(dbounov)?

@@ +614,5 @@
> +    MOZ_ASSERT(isWasm());
> +    uint8_t* data = dataPointer();
> +
> +    if (!data)
> +        return false;

I think this test can be removed (as in mappedSize()).

@@ +626,5 @@
> +    MOZ_RELEASE_ASSERT(((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);
> +    MOZ_RELEASE_ASSERT(
> +            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);
> +    MOZ_RELEASE_ASSERT(
> +            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize <= (uint64_t) mappedSize());

We assert here that the current size + the delta is within the mapped size.  However, in Instance::growMemory() we've only guarded against metadata.maxMemorySize() which is possibly greater than the actual mapped size we were able to allocate in createForWasm.  If that's all right, could you add a test to catch this case?

@@ +635,5 @@
> +
> +    MOZ_RELEASE_ASSERT(newSize <= UINT32_MAX && newSize <= mappedSize());
> +
> +# ifdef XP_WIN
> +    // TODO(dbounov) Is the below XP code correct for growing memory?

Looks like it.  Or you could use VirtualProtect() which seems more scoped to this task.

@@ +649,5 @@
> +#  endif
> +
> +    MemProfiler::SampleNative(dataEnd, deltaBytes);
> +    // TODO(dbounov) read-only field. Need help from Luke to fix this
> +    setByteLength(newSize);

For now, let's not change byteLength at all.  Thus, wasm will be able to get to it via loads/stores, but outside JS won't.  That's actually the correct semantics: the missing step is that we need to create a new ArrayBuffer with the new, bigger byteLength that aliases the same memory and change WebAssembly.Memory.buffer to return that instead (and, if max isn't specified, detach the old ArrayBuffer).  I can do that patch when we land this one.

The reason for this conservativity is that it's possible SM impl code somewhere/somehow depends on byteLength staying constant.  Can you also remove setByteLength() methods everywhere?

::: js/src/vm/ArrayBufferObject.h
@@ +99,5 @@
>  typedef Handle<ArrayBufferObjectMaybeShared*> HandleArrayBufferObjectMaybeShared;
>  typedef MutableHandle<ArrayBufferObjectMaybeShared*> MutableHandleArrayBufferObjectMaybeShared;
>  
> +class ArrayRawBufferObject {
> +  private:

nit: { on new line, no need for 'private' since class

@@ +112,5 @@
> +    }
> +
> +  public:
> +    static ArrayRawBufferObject* New(uint8_t* p, uint32_t length, size_t mappedSize)
> +    {

nit: { on preceding line

@@ +123,5 @@
> +    static void* AllocateWasmMappedMemory(uint32_t numBytes, size_t mappedSize);
> +    static void ReleaseWasmMappedMemory(void* mem);
> +
> +    uint8_t* dataPointer() {
> +        uint8_t* ptr = reinterpret_cast<uint8_t*>(const_cast<ArrayRawBufferObject*>(this));

Don't need const_cast anymore, I think

::: js/src/vm/SharedArrayObject.cpp
@@ -72,5 @@
>      return true;
>  #endif
>  }
>  
> -#if defined(ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB)

I think we need to keep this WASM_HUGE_MEMORY: otherwise on 32-bit we'll be trying to map 4gb.  (I wonder how this doesn't fail SAB tests on 32-bit.)

::: js/src/vm/SharedArrayObject.h
@@ +45,5 @@
>  {
>    private:
>      mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> refcount_;
>      uint32_t length;
> +    size_t mappedSize_;

Could you remove all the SharedArrayObject.h changes?  SAB can only be accessed through asm.js and thus it won't need to grow or use current_memory.  Before we have wasm+threads, I think there were going to be some significant changes to how this is represented anyway, so I think best just to leave it the same.  That means:
 - removing the AnyArray* functions and the methods from ArrayBufferObjectMaybeShared
 - MOZ_RELEASE_ASSERT()ing you have an ArrayBuffer in Instance::{growMemory,currentMemory} et al
Comment on attachment 8780364 [details] [diff] [review]
grow_mem_candidate_v13.diff

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

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +6982,2 @@
>  #if defined(JS_CODEGEN_X64)
> +    // We rely on signals to interrupt code.

This comment is redundant with the comment immediately above.

::: js/src/asmjs/WasmModule.cpp
@@ +520,5 @@
>      if (memory) {
>          buffer = &memory->buffer();
>          uint32_t length = buffer->byteLength();
> +
> +        // Its not an error to import a memory, whose mapped size is less than

Grammar-in-a-comment nit :-): "It's" rather than "Its".

::: js/src/builtin/TestingFunctions.cpp
@@ +3840,5 @@
>  "  Returns a boolean indicating whether WebAssembly is supported on the current device."),
>  
>      JS_FN_HELP("wasmUsesSignalForOOB", WasmUsesSignalForOOB, 0, 0,
>  "wasmUsesSignalForOOB()",
>  "  Return whether wasm and asm.js use a signal handler for detecting out-of-bounds."),

Is this option, wasmUsesSignalForOOB, still needed, now that we're requiring signals?

::: js/src/jit-test/tests/wasm/basic-memory.js
@@ +155,3 @@
>      if (ind == 1)
>          setJitCompilerOption('wasm.explicit-bounds-checks', 1);
> +    */

Could you add a comment here explaining what this code is for?

::: js/src/vm/ArrayBufferObject.cpp
@@ +400,5 @@
>                                          MAP_PRIVATE | MAP_ANON, -1, 0, "wasm-reserved");
>      if (data == MAP_FAILED)
>          return nullptr;
>  
> +    // TODO(dbounov): Note we will waste a page on zero-sized memories here

(As with Luke's comment on the other copy of this comment, we can drop the TODO here.)
Hrm.

At a skim this patch *may* not conflict with sfink, who has several outstanding ArrayBuffer patches to land (and for various reasons aren't can quite land immediately).  At least, it looks like it might not in terms of code touched.

But if there does happen to be any conflict conceptually (I'm not prepared to evaluate this at a skim), that really, really, really want to land before this does.  Please coordinate with sfink somehow on this?
(Assignee)

Comment 22

2 years ago
Created attachment 8780787 [details] [diff] [review]
grow_mem_candidate_v19.diff

Another day another patch:

1) Addressed Dan and Luke's comments from last round (see inline responses for detail)

2) Merged AstNop into AstNullaryOperator

3) Removed as much as possible of the changes in SharedArrayObject.{cpp,h}

Special attention should be given to 
   - the changes in SharedArrayObject.{cpp,h}, 
   - the changes in MacroAssembler and Baseline and Ion related to emitting builtin instance methods, 
   - the merging of AstNop.
   - the use of VirtualProtect in growForWasm

Detailed response:

(In reply to Luke Wagner [:luke] from comment #19)
> grow_mem_candidate_v13.diff
> 
> Review of attachment 8780364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, this is *almost* ready to land.  Almost all nits, but I think I found
> one bug, as well as that one test that it'd be good to reenable, that'd I'd
> like to understand and see addressed in a new patch.
> 
> ::: js/src/asmjs/AsmJS.cpp
> @@ +8811,3 @@
> >  static const size_t MinHeapLength = PageSize;
> > +// The asm.js valid heap lengths are precisely the WASM valid heap lengths for ARM
> > +// greater or equal to MinHeapLength
> 
> nit: could you put MinHeapLength under the comment then with a \n after
> MinHeapLength?

Fixed.

> 
> ::: js/src/asmjs/WasmAST.h
> @@ +833,5 @@
> >          return globals_;
> >      }
> >  };
> >  
> > +// TODO(dbounov) Should I merge AstNop into this?
> 
> Sure (or, if not, remove the TODO before landing).
> 

Fixed - merged AstNop.

> ::: js/src/asmjs/WasmCode.cpp
> @@ +114,5 @@
> > +#else
> > +    // On x64 with 4G guard regions we still might get some BCs due to atomics.
> > +    // For those check up to the maxMemoryLength. This should be inside the 4G
> > +    // guard region.
> > +    uint32_t bc_limit = metadata.maxMemoryLength.valueOr(metadata.minMemoryLength);
> 
> nit: we don't usually name with _ in SM; how about just 'length'?
> 

Fixed.

> @@ +610,5 @@
> >  
> >      return &metadata_->codeRanges[match];
> >  }
> >  
> > +#ifdef JS_CODEGEN_X64
> 
> WASM_HUGE_MEMORY (basically, here and every other place in the patch that
> says JS_CODEGEN_X64)
> 

Changed on all that I added except passInstance() (since there its about the size of the register)

> ::: js/src/asmjs/WasmCode.h
> @@ +416,5 @@
> >    public:
> >      ModuleKind            kind;
> >      MemoryUsage           memoryUsage;
> >      uint32_t              minMemoryLength;
> > +    mozilla::Maybe<uint32_t> maxMemoryLength;
> 
> nit: b/c of the 'using mozilla::Maybe' in WasmTypes.h, you shouldn't need
> the mozilla:: here.
> 

Fixed.

> ::: js/src/asmjs/WasmCompile.cpp
> @@ +1600,5 @@
> >  SharedModule
> >  wasm::Compile(const ShareableBytes& bytecode, CompileArgs&& args, UniqueChars* error)
> >  {
> >      bool newFormat = args.assumptions.newFormat;
> > +    MOZ_RELEASE_ASSERT(wasm::SignalHandlersAlreadyInstalled());
> 
> As a precondition, can you put it first with a \n after?

Fixed.

> 
> ::: js/src/asmjs/WasmGenerator.h
> @@ +53,3 @@
> >      MemoryUsage               memoryUsage;
> >      mozilla::Atomic<uint32_t> minMemoryLength;
> > +    mozilla::Maybe<uint32_t>  maxMemoryLength;
> 
> nit: shouldn't need the mozilla:: here
> 

Fixed.

> ::: js/src/asmjs/WasmInstance.cpp
> @@ +285,5 @@
> > +
> > +uint32_t
> > +Instance::growMemory(uint32_t delta)
> > +{
> > +    MOZ_RELEASE_ASSERT(memory_);
> 
> nit: \n after

Fixed.

> 
> @@ +287,5 @@
> > +Instance::growMemory(uint32_t delta)
> > +{
> > +    MOZ_RELEASE_ASSERT(memory_);
> > +    // Using uint64_t to avoid worrying about overflows in safety comp.
> > +    uint64_t curNPages = currentMemory();
> 
> nit: we generally spell out "num" instead of "n" in names
> 

Fixed.

> @@ +292,5 @@
> > +    uint64_t newNPages = curNPages + (uint64_t) delta;
> > +    uint64_t maxMemoryByteLength =
> > +        metadata().maxMemoryLength.valueOr(metadata().minMemoryLength);
> > +
> > +    if (newNPages * wasm::PageSize > maxMemoryByteLength)
> 
> Can we express this logic instead as:
>   if (metadata().maxMemoryLength) {
>      if (beyond mappedSize)
>         return -1;
>      if (!memory_->buffer().growForWasm(delta))
>         return -1;
>   } else {
>      return -1;  // TODO: implement grow_memory without max
>   }
> ?  That way we clear out where we'll do something totally different for the
> !max case.
> 

Fixed.

> ::: js/src/asmjs/WasmIonCompile.cpp
> @@ +886,5 @@
> > +
> > +        // Should only pass an instance once.
> > +        MOZ_ASSERT(args->instanceArg_ == ABIArg());
> > +        ValType ptrType = (sizeof(void*) == 4 ? ValType::I32 : ValType::I64);
> > +        args->instanceArg_ = args->abi_.next(ToMIRType(ptrType));
> 
> You can replace this with MIRType::Pointer.
> 

Fixed.

> @@ +1099,5 @@
> > +        CallSiteDesc desc(call.lineOrBytecode_, CallSiteDesc::Register);
> > +        auto callee = CalleeDesc::builtinInstanceMethod(builtin);
> > +        auto* ins = MWasmCall::NewBuiltinInstanceMethodCall(alloc(), desc, callee,
> > +                call.instanceArg_, call.regArgs_, ToMIRType(ret), call.spIncrement_,
> > +                MWasmCall::DontSaveTls);
> 
> nit: SM indentation style lines up args with opening ( of call.
> 

Fixed.

> ::: js/src/asmjs/WasmModule.cpp
> @@ +17,5 @@
> >   */
> >  
> >  #include "asmjs/WasmModule.h"
> >  
> > +#include "jsutil.h"
> 
> #include still necessary?
> 

Removed.

> @@ +524,5 @@
> > +        // Its not an error to import a memory, whose mapped size is less than
> > +        // the maxMemoryLength required for the module. This is the same as trying to
> > +        // map up to maxMemoryLength but actually getting less.
> > +        if (length < metadata_->minMemoryLength ||
> > +            length > metadata_->maxMemoryLength.valueOr(UINT32_MAX)) {
> 
> Can you hoist a
>   uint32_t maxMemoryLength = metadata_->maxMemoryLength.valueOr(UINT32_MAX);
> above the if (with \n before 'buffer') so that you can reuse here and below
> in LegalizeMemoryLength(), keeping both expressions on a single line?
> 

Not a nissue - the two expressions are actually different and cannot be hoisted.

> ::: js/src/asmjs/WasmSignalHandlers.cpp
> @@ +801,5 @@
> >          return pc == instance->codeSegment().interruptCode() &&
> >                 instance->codeSegment().containsFunctionPC(activation->resumePC()) &&
> >                 instance->code().lookupMemoryAccess(activation->resumePC());
> > +#else
> > +        // TODO(dbounov): Is this safe??
> 
> Yes, I think so: the first two conditions are sufficient to identify this
> peculiar situation.  In fact, I'd remove the #ifdefs and lookupMemoryAccess
> test entirely (just so platforms behave the same).
> 

Fixed - ifdefs Removed.

> @@ +818,5 @@
> >      *ppc = EmulateHeapAccess(context, pc, faultingAddress, memoryAccess, *instance);
> > +#elif defined(JS_CODEGEN_X86)
> > +    *ppc = instance->codeSegment().outOfBoundsCode();
> > +#else // ARM
> > +    MOZ_CRASH("Unsupported architecture for windows.");
> 
> I don't think we have to worry about unaligned vs. out-of-bounds
> on-ARM-on-Windows; can just have all !WASM_HUGE_MEMORY redirect pc to
> outOfBoundsCode?
> 

Fixed.

> @@ +954,5 @@
> >      *ppc = EmulateHeapAccess(&context, pc, faultingAddress, memoryAccess, *instance);
> > +#elif defined(JS_CODEGEN_X86)
> > +    *ppc = instance->codeSegment().outOfBoundsCode();
> > +#else // ARM
> > +    MOZ_CRASH("Unsupported architecture for MacOS X.");
> 
> ditto about having having all !WASM_HUGE_MEMORY redirect to outOfBoundsCode.
> 

Fixed.

> @@ +1181,5 @@
> > +#elif defined(JS_CODEGEN_X86)
> > +    *ppc = instance->codeSegment().outOfBoundsCode();
> > +#elif defined(JS_CODEGEN_ARM)
> > +    if (signal == Signal::BusError)
> > +        *ppc = instance->codeSegment().unalignedAccessCode();
> 
> nit: SM style says to put { } around this since the else has {}s.  Or, you
> could
>   MOZ_RELEASE_ASSERT(signal == Signal::BusError || signal ==
> Signal::SegFault);
> and neither branch needs {} :)
> 

Fixed.

> @@ +1187,5 @@
> > +        MOZ_RELEASE_ASSERT(signal == Signal::SegFault);
> > +        *ppc = instance->codeSegment().outOfBoundsCode();
> > +    }
> > +#else
> > +    MOZ_CRASH("Unsupported architecture for Linux.");
> 
> ditto about having having all !WASM_HUGE_MEMORY redirect to outOfBoundsCode.
> 

On this one I still check wether we are JS_CODEGEN_ARM or JS_CODEGEN_X86 in the #else.

> @@ +1412,4 @@
> >  }
> >  
> > +bool
> > +wasm::SignalHandlersAlreadyInstalled()
> 
> I think we don't need a new function here: wasm::HaveSignalHandlers() is
> only called after JSRuntime::init() which calls EnsureSignalHandlers() so we
> can:
>  - pull the statics sTried and sResult out of ProcessHasSignalHandlers() so
> that wasm::SignalHandlers() can `MOZ_ASSERT(sTried); return sResult;`.
>  - probably good to rename to
> sTriedToInstallSignalHandlers/sInstalledSignalHandlers
> 

Fixed. But I can't find "TriedToInstallSignalHandlers"

> ::: js/src/asmjs/WasmTypes.cpp
> @@ +636,5 @@
> > +        length = mozilla::RoundUpPow2(length);
> > +    else
> > +        length = (length + 0x00ffffff) & ~0x00ffffff;
> > +
> > +    MOZ_ASSERT(length % PageSize == 0);
> 
> Or perhaps MOZ_ASSERT(IsvalidARMLengthImmedaite(length))?
> 

Fixed.

> ::: js/src/asmjs/WasmTypes.h
> @@ +1236,5 @@
> >  };
> >  
> > +#ifdef JS_CODEGEN_X64
> > +#define WASM_HUGE_MEMORY
> > +#endif
> 
> Perhaps move this down and put the Uint32Range/MappedRange consts inside
> this #ifdef to show the association?
> 

Fixed.

> ::: js/src/builtin/TestingFunctions.cpp
> @@ +517,5 @@
> >  static bool
> >  WasmUsesSignalForOOB(JSContext* cx, unsigned argc, Value* vp)
> >  {
> >      CallArgs args = CallArgsFromVp(argc, vp);
> > +    args.rval().setBoolean(true);
> 
> Could you remove this function and dependent tests too?
> 

Fixed.

> ::: js/src/jit-test/tests/asm.js/testHeapAccess.js
> @@ +27,5 @@
> >  
> > +// TODO(dbounov): Root-cause why this fails
> > +// TODO(dbounov): Is this test still relevant?
> > +// Bug 1088655
> > +//assertEq(asmLink(asmCompile('stdlib', 'foreign', 'heap', USE_ASM + 'var i32=new stdlib.Int32Array(heap); function f(i) {i=i|0;var j=0x10000;return (i32[j>>2] = i)|0 } return f'), this, null, buf)(1), 1);
> 
> It does seem like this should pass; can you enable this test?
> 

Fixed - I had copied only part of the old code when refactoring.

> ::: js/src/jit-test/tests/wasm/basic-grow-memory.js
> @@ +90,5 @@
> > +assertEq(linearModule(1,2,[["GM", 0, 1], [op, "i32", "", "0", "65532"]])(), 1)
> > +assertOOB(()=>linearModule(1,2,[["GM", 0, 1], [op, "i32", "", "0", "65536"]])())
> > +
> > +// Test that if we don't specify a maximum memory size, we can't grow.
> > +// Note that this should change on both x64 and x86 when single threaded.
> 
> We need growing to work on all archs, not just x86/x64.
> 

Fixed.

> ::: js/src/jit/MIR.cpp
> @@ +5389,5 @@
> > +    MWasmCall* call = MWasmCall::New(alloc, desc, callee, args, resultType, spIncrement,
> > +                                     tlsStackOffset, nullptr);
> > +
> > +    // instanceArg should be specified ONLY for CalleeDesc::BuiltinInstanceMethod
> > +    MOZ_ASSERT(callee.which() == wasm::CalleeDesc::BuiltinInstanceMethod);
> 
> How about having NewBuiltinInstanceMethodCall take a SymbolicAddress instead?
> 

Fixed.

> ::: js/src/jit/MIR.h
> @@ +13555,5 @@
> > +    static MWasmCall* NewBuiltinInstanceMethodCall(TempAllocator& alloc,
> > +                          const wasm::CallSiteDesc& desc,
> > +                          const wasm::CalleeDesc& callee, const ABIArg& instanceArg,
> > +                          const Args& args, MIRType resultType,
> > +                          uint32_t spIncrement, uint32_t tlsStackOffset);
> 
> You can remove the tlsStackOffset argument (it's for wasm->wasm calls) and
> pass DontSaveTls internally.
> 

Fixed.

> ::: js/src/jit/MIRGraph.cpp
> @@ +113,5 @@
> >      // We use signal-handlers on x64, but on x86 there isn't enough address
> >      // space for a guard region.  Also, on x64 the atomic loads and stores
> >      // can't (yet) use the signal handlers.
> > +#ifdef WASM_HUGE_MEMORY
> > +    if (!access->isAtomicAccess())
> 
> In some later patch, it'd be nice to remove these methods from MIRGenerator
> (now that they don't depend on MIRGenerator state) and make them methods of
> MWasmMemoryAccess.

TODO: - need to file a bug for this.

> 
> ::: js/src/jit/RegisterSets.h
> @@ +1272,5 @@
> >      bool argInRegister() const { return kind() != Stack; }
> >      AnyRegister reg() const { return kind_ == GPR ? AnyRegister(gpr()) : AnyRegister(fpu()); }
> > +
> > +    bool operator==(const ABIArg& rhs) const {
> > +        if (kind_ != rhs.kind_) return false;
> 
> nit: return on new line
> 

Fixed.

> ::: js/src/jit/shared/CodeGenerator-shared.cpp
> @@ +1516,5 @@
> >      const wasm::CallSiteDesc& desc = mir->desc();
> >      const wasm::CalleeDesc& callee = mir->callee();
> > +
> > +    // If this is a builtin instance method call, we must have
> > +    // reserved the first argument for passing in Instance*. Pass it in now.
> 
> Can you factor out this code that is common between Ion and Baseline into a
> wasmBuiltinMethodCall(SymbolicAddress) method in MacroAssembler.(h,cpp)
> similar to wasmCallImport/Indirect?
> 

Fixed.

> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +390,4 @@
> >      if (!data)
> >          return nullptr;
> >  
> > +    // TODO(dbounov): Note we will waste a page on zero-sized memories here
> 
> Comment makes sense to keep; can you remove the TODO(dbounov)?
> 

Fixed.

> @@ +614,5 @@
> > +    MOZ_ASSERT(isWasm());
> > +    uint8_t* data = dataPointer();
> > +
> > +    if (!data)
> > +        return false;
> 
> I think this test can be removed (as in mappedSize()).
> 

Fixed.

> @@ +626,5 @@
> > +    MOZ_RELEASE_ASSERT(((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);
> > +    MOZ_RELEASE_ASSERT(
> > +            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);
> > +    MOZ_RELEASE_ASSERT(
> > +            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize <= (uint64_t) mappedSize());
> 
> We assert here that the current size + the delta is within the mapped size. 
> However, in Instance::growMemory() we've only guarded against
> metadata.maxMemorySize() which is possibly greater than the actual mapped
> size we were able to allocate in createForWasm.  If that's all right, could
> you add a test to catch this case?
> 

Nope, thats a bug. In growMemory() we should guard against Min(mappedSize, maxMemory).
Fixed.

> @@ +635,5 @@
> > +
> > +    MOZ_RELEASE_ASSERT(newSize <= UINT32_MAX && newSize <= mappedSize());
> > +
> > +# ifdef XP_WIN
> > +    // TODO(dbounov) Is the below XP code correct for growing memory?
> 
> Looks like it.  Or you could use VirtualProtect() which seems more scoped to
> this task.
> 

Fixed - Changed to VirtualProtect.

> @@ +649,5 @@
> > +#  endif
> > +
> > +    MemProfiler::SampleNative(dataEnd, deltaBytes);
> > +    // TODO(dbounov) read-only field. Need help from Luke to fix this
> > +    setByteLength(newSize);
> 
> For now, let's not change byteLength at all.  Thus, wasm will be able to get
> to it via loads/stores, but outside JS won't.  That's actually the correct
> semantics: the missing step is that we need to create a new ArrayBuffer with
> the new, bigger byteLength that aliases the same memory and change
> WebAssembly.Memory.buffer to return that instead (and, if max isn't
> specified, detach the old ArrayBuffer).  I can do that patch when we land
> this one.
> 
> The reason for this conservativity is that it's possible SM impl code
> somewhere/somehow depends on byteLength staying constant.  Can you also
> remove setByteLength() methods everywhere?
> 

Fixed. I only removed the setByteLength's that I added. There were some setByteLengths already there used internally for initialization. I left those as is.

> ::: js/src/vm/ArrayBufferObject.h
> @@ +99,5 @@
> >  typedef Handle<ArrayBufferObjectMaybeShared*> HandleArrayBufferObjectMaybeShared;
> >  typedef MutableHandle<ArrayBufferObjectMaybeShared*> MutableHandleArrayBufferObjectMaybeShared;
> >  
> > +class ArrayRawBufferObject {
> > +  private:
> 
> nit: { on new line, no need for 'private' since class
> 

Fixed.

> @@ +112,5 @@
> > +    }
> > +
> > +  public:
> > +    static ArrayRawBufferObject* New(uint8_t* p, uint32_t length, size_t mappedSize)
> > +    {
> 
> nit: { on preceding line

Fixed

> 
> @@ +123,5 @@
> > +    static void* AllocateWasmMappedMemory(uint32_t numBytes, size_t mappedSize);
> > +    static void ReleaseWasmMappedMemory(void* mem);
> > +
> > +    uint8_t* dataPointer() {
> > +        uint8_t* ptr = reinterpret_cast<uint8_t*>(const_cast<ArrayRawBufferObject*>(this));
> 
> Don't need const_cast anymore, I think
> 

Fixed.

> ::: js/src/vm/SharedArrayObject.cpp
> @@ -72,5 @@
> >      return true;
> >  #endif
> >  }
> >  
> > -#if defined(ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB)
> 
> I think we need to keep this WASM_HUGE_MEMORY: otherwise on 32-bit we'll be
> trying to map 4gb.  (I wonder how this doesn't fail SAB tests on 32-bit.)
> 

Fixed.

> ::: js/src/vm/SharedArrayObject.h
> @@ +45,5 @@
> >  {
> >    private:
> >      mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> refcount_;
> >      uint32_t length;
> > +    size_t mappedSize_;
> 
> Could you remove all the SharedArrayObject.h changes?  SAB can only be
> accessed through asm.js and thus it won't need to grow or use
> current_memory.  Before we have wasm+threads, I think there were going to be
> some significant changes to how this is represented anyway, so I think best
> just to leave it the same.  That means:
>  - removing the AnyArray* functions and the methods from
> ArrayBufferObjectMaybeShared
>  - MOZ_RELEASE_ASSERT()ing you have an ArrayBuffer in
> Instance::{growMemory,currentMemory} et al

Fixed - reverted as many of the changes as possible, hacked up the mappedSize for SAB in ArrayBufferObject-inl.h
Attachment #8780364 - Attachment is obsolete: true
Attachment #8780364 - Flags: review?(sunfish)
Attachment #8780364 - Flags: review?(luke)
Attachment #8780787 - Flags: review?(sunfish)
Attachment #8780787 - Flags: review?(luke)
(Assignee)

Comment 23

2 years ago
Created attachment 8781199 [details] [diff] [review]
grow_mem_candidate_v21.diff

Sorry, for the noise. Two small nits with the last patch:

dimo@scruffy:~/work/mozilla-memory-5/js/src$ diff ../../../grow_mem_candidate_v19.diff ../../../grow_mem_c
andidate_v21.diff                                                                                         
649c649
< @@ -1079,16 +1090,19 @@ AstDecodeExpr(AstDecodeContext& c)
---
> @@ -1079,16 +1090,20 @@ AstDecodeExpr(AstDecodeContext& c)
660a661
> +        break;

Typo/bug - missing break

4341c4342
< @@ -589,23 +587,85 @@ ArrayBufferObject::setDataPointer(Buffer
---
> @@ -589,23 +587,84 @@ ArrayBufferObject::setDataPointer(Buffer
4400,4401c4401
< +    uint32_t oldFlags;
< +    if (deltaBytes && !VirtualProtect(dataEnd, deltaBytes, PAGE_READWRITE, &oldFlags))
---
> +    if (deltaBytes && !VirtualAlloc(dataEnd, deltaBytes, MEM_COMMIT, PAGE_READWRITE))

Reverted back to VirtualAlloc. So according to MSDN VirtualProtect works only if all pages have been comitted. 

https://msdn.microsoft.com/en-us/library/windows/desktop/aa366898(v=vs.85).aspx

4427c4427
< @@ -630,20 +690,21 @@ ArrayBufferObject::create(JSContext* cx,
---
> @@ -630,20 +689,21 @@ ArrayBufferObject::create(JSContext* cx,
Attachment #8780787 - Attachment is obsolete: true
Attachment #8780787 - Flags: review?(sunfish)
Attachment #8780787 - Flags: review?(luke)
Attachment #8781199 - Flags: review?(sunfish)
Attachment #8781199 - Flags: review?(luke)
Comment on attachment 8781199 [details] [diff] [review]
grow_mem_candidate_v21.diff

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

This looks good to me, with just a few more style nits.

::: js/src/asmjs/WasmCode.h
@@ +519,5 @@
>      // Frame iterator support:
>  
>      const CallSite* lookupCallSite(void* returnAddress) const;
>      const CodeRange* lookupRange(void* pc) const;
> +#ifdef WASM_HUGE_MEMORY 

Nit: trailing whitespace.

::: js/src/asmjs/WasmGenerator.h
@@ +53,3 @@
>      MemoryUsage               memoryUsage;
>      mozilla::Atomic<uint32_t> minMemoryLength;
> +    Maybe<uint32_t>  maxMemoryLength;

Style nit: since the other members are indented to a common column, maxMemoryLength should be too.

::: js/src/asmjs/WasmTypes.h
@@ +807,5 @@
>  };
>  
>  // Summarizes a heap access made by wasm code that needs to be patched later
>  // and/or looked up by the wasm signal handlers. Different architectures need
> +// to know different things (x64: offset and length, ARM: nothing, x86: offset of end of

Nit: SM style recommends wrapping comments at 80 columns.
Attachment #8781199 - Flags: review?(sunfish) → review+
(Assignee)

Comment 25

2 years ago
Created attachment 8781280 [details] [diff] [review]
grow_mem_candidate_v22.diff

Not cancelling previous patch (grow_mem_candidate_v21.diff) for review since all changes here are whitespace. All of dan's style nits fixed.

(In reply to Dan Gohman [:sunfish] from comment #24)
> Comment on attachment 8781199 [details] [diff] [review]
> grow_mem_candidate_v21.diff
> 
> Review of attachment 8781199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me, with just a few more style nits.
> 
> ::: js/src/asmjs/WasmCode.h
> @@ +519,5 @@
> >      // Frame iterator support:
> >  
> >      const CallSite* lookupCallSite(void* returnAddress) const;
> >      const CodeRange* lookupRange(void* pc) const;
> > +#ifdef WASM_HUGE_MEMORY 
> 
> Nit: trailing whitespace.
> 

Fixed

> ::: js/src/asmjs/WasmGenerator.h
> @@ +53,3 @@
> >      MemoryUsage               memoryUsage;
> >      mozilla::Atomic<uint32_t> minMemoryLength;
> > +    Maybe<uint32_t>  maxMemoryLength;
> 
> Style nit: since the other members are indented to a common column,
> maxMemoryLength should be too.
> 

Fixed.

> ::: js/src/asmjs/WasmTypes.h
> @@ +807,5 @@
> >  };
> >  
> >  // Summarizes a heap access made by wasm code that needs to be patched later
> >  // and/or looked up by the wasm signal handlers. Different architectures need
> > +// to know different things (x64: offset and length, ARM: nothing, x86: offset of end of
> 
> Nit: SM style recommends wrapping comments at 80 columns.

Fixed.

Comment 26

2 years ago
Comment on attachment 8781199 [details] [diff] [review]
grow_mem_candidate_v21.diff

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

Looking very good, but one last close sweep found a few non-nit issues, so probably need one more pass:

::: js/src/asmjs/WasmCode.cpp
@@ +111,5 @@
> +#ifndef WASM_HUGE_MEMORY
> +    static_assert(sizeof(uint32_t) == sizeof(size_t), "Must be 32 bit if we don't have HUGE_MEMORY");
> +    uint32_t length = memory->buffer().mappedSize();
> +#else
> +    // On x64 with 4G guard regions we still might get some BCs due to atomics.

nit: I'd spell out "bounds checks" in this comment.

@@ +114,5 @@
> +#else
> +    // On x64 with 4G guard regions we still might get some BCs due to atomics.
> +    // For those check up to the maxMemoryLength. This should be inside the 4G
> +    // guard region.
> +    uint32_t length = metadata.maxMemoryLength.valueOr(metadata.minMemoryLength);

In theory, atomics don't support faulting *at all*.  Thus, in both the WASM_HUGE_MEMORY and not cases, using mappedSize or maxMemoryLength is invalid: it'll allow atomics to fault!  However, I'm thinking now that it's perfectly fine for atomics to fault: the original reason to disallow was because atomics started with the bad asm.js non-throwing semantics and this was hard to implement; but now they have throwing semantics so I think the signal handling machinery just works (apparently, assuming our tests are hitting this!).

So it seems like we should just remove all the isAtomic() special cases and stop pretending we're not faulting on atomic accesses.

With that, I think we can unconditionally say 'length = memory->buffer().mappedSize()'.  (Which also seems necessary since otherwise aren't we feeding in un-legalized length values?)  It'd be good to add a MOZ_RELEASE_ASSERT(length == LegalizeMemoryLength(length)).

::: js/src/asmjs/WasmGenerator.h
@@ +53,3 @@
>      MemoryUsage               memoryUsage;
>      mozilla::Atomic<uint32_t> minMemoryLength;
> +    Maybe<uint32_t>  maxMemoryLength;

nit: can you align 'max' with 'min'?

::: js/src/asmjs/WasmInstance.cpp
@@ +296,5 @@
> +        // The maxMemoryLength can be both greather than mappedSize (if we don't have enough
> +        // address space) and smaller than (due to rounding/slop pages).
> +        // So we need to check against the minimum of the two to be safe.
> +        size_t maxGrowLength = Min(((size_t)*(metadata().maxMemoryLength)),
> +                                   memory_->buffer().mappedSize());

So I think this isn't quite sufficient: many instances could be importing the same memory and we have to respect all of their maxes.  Thus, it's not sufficient to compare only against *this* module's static maxMemoryLength.  (I think the design docs need improvement here.)  Rather than attempting to maintain a linked list of linked instances, I think what we need to do is this: every time we instantiate a module against a memory (so in Module::instantiateMemory(), we take the Min() of the current max and the newly-instantiated module's maxMemoryLength.  Because legalization necessarily adds slop which we must maintain, I think this maxMemoryLength needs to be a separate field from mappedSize with the invariant buffer.maxMemoryLength <= buffer.mappedSize.  With that new field, you can simply check against buffer().maxMemoryLength() here.  To wit: we can unmap mapped memory if Legalize(buffer.maxMemoryLength) < buffer.mappedSize.

Also, nit: I think you can Min<size_t>() instead of casting the argument.

::: js/src/asmjs/WasmJS.cpp
@@ +762,5 @@
>          return false;
>      }
>  
>      uint32_t bytes = uint32_t(initialDbl) * PageSize;
> +    size_t mappedSize = LegalizeMemoryLength(bytes);

Could you add a TODO here for using the 'maximum' property instead?  (or just add it :)

::: js/src/asmjs/WasmModule.cpp
@@ +518,5 @@
>      if (memory) {
>          buffer = &memory->buffer();
>          uint32_t length = buffer->byteLength();
> +
> +        // It's not an error to import a memory, whose mapped size is less than

nit: remove ,

::: js/src/asmjs/WasmSignalHandlers.cpp
@@ +605,5 @@
>  EmulateHeapAccess(EMULATOR_CONTEXT* context, uint8_t* pc, uint8_t* faultingAddress,
>                    const MemoryAccess* memoryAccess, const Instance& instance)
>  {
>      MOZ_RELEASE_ASSERT(instance.codeSegment().containsFunctionPC(pc));
> +    MOZ_RELEASE_ASSERT(wasm::HaveSignalHandlers());

Can remove since in signal handler :)

@@ +802,4 @@
>      }
>  
> +    MOZ_RELEASE_ASSERT(instance->codeSegment().containsFunctionPC(pc));
> +    MOZ_RELEASE_ASSERT(wasm::HaveSignalHandlers());

For the former assert, I think we should 'return false' if !containsFunctionPC.  That means we'll crash at the faulting insn which is a bit nicer when debugging.  For the latter, I'm not sure it adds much value since we're in the signal handler :)  Ditto for the OSX and Unix copies below.

@@ +1170,5 @@
> +#else
> +# if defined(JS_CODEGEN_X86)
> +    MOZ_RELEASE_ASSERT(signal == Signal::SegFault);
> +    *ppc = instance->codeSegment().outOfBoundsCode();
> +# else // JS_CODEGEN_ARM

Since BusError is only installed on ARM, how about flipping the #else around so it's:

#if defined(WASM_HUGE_MEMORY)
MOZ_RELEASE_ASSERT(signal == Signal::SegFault);
...
#elif defined(JS_CODEGEN_ARM)
MOZ_RELEASE_ASSERT(signal == Signal::BusError || signal == Signal::SegFault);
...
#else
MOZ_RELEASE_ASSERT(signal == Signal::SegFault);
...
#endif

@@ +1172,5 @@
> +    MOZ_RELEASE_ASSERT(signal == Signal::SegFault);
> +    *ppc = instance->codeSegment().outOfBoundsCode();
> +# else // JS_CODEGEN_ARM
> +    MOZ_RELEASE_ASSERT(signal == Signal::BusError || signal == Signal::SegFault);
> +

nit: can you remove \n for symmetry with above?

@@ +1282,5 @@
>  }
>  #endif
>  
> +static bool sTried = false;
> +static bool sResult = false;

Sorry I wasn't more clear earlier, I just meant to rename these to sTriedInstallSignalHandlers/sHaveSignalHandlers, resp.

::: js/src/jit/MacroAssembler.h
@@ +1330,5 @@
>  
>      // WasmTableCallIndexReg must contain the index of the indirect call.
>      void wasmCallIndirect(const wasm::CallSiteDesc& desc, const wasm::CalleeDesc& callee);
>  
> +    void wasmCallBuiltinInstanceMethod(const ABIArg& instanceArg,

For symmetry with preceding comments, probably good to be clear that this method implicitly preserves TLS and pinned regs.

::: js/src/vm/ArrayBufferObject.cpp
@@ +383,1 @@
>      MOZ_ASSERT(numBytes % wasm::PageSize == 0);

Can you also assert mappedSize % gc::SystemPageSize() == 0?

@@ +383,4 @@
>      MOZ_ASSERT(numBytes % wasm::PageSize == 0);
>  
> +    uint64_t actualMappedSize = mappedSize + gc::SystemPageSize();
> +    uint64_t actualNumBytes = numBytes + gc::SystemPageSize();

Unfortunately, now we have two meanings for "actual".  Maybe name to mappedSizeWithHeader/numBytesWithHeader?

@@ +452,5 @@
> +        // If fail, apply binary search for the maximum allocatable chunk in the range
> +        // [wasm::legalizeMappedSize(numBytes), mapSize-1]
> +        size_t min = wasm::LegalizeMemoryLength(numBytes), max = mapSize;
> +
> +        MOZ_RELEASE_ASSERT(numBytes <= min && min <= max && mapSize <= max);

The last conjunct doesn't seem to add anything given preceding init.

@@ +479,1 @@
>          if (!data) {

I worry about the race condition where an allocation succeeds at size N, we release it, iterate, exit the loop, then try allocate it again, but now the allocation fails (due to race) and we throw.  I can see this happening if a site loads N workers, each with a large-ish max.  If we had Linux's mremap() everywhere, then we wouldn't have to release the memory; we could mremap() in-place.  Thinking through a couple of different variations, the best heuristic I can think of that avoids this race is:

   void* p = null
   for (n = mappedSize; n >= minSize; n = clamp(legalize(n / 2), minSize))
     p = allocate n
     if (p) break;
   if (!p) fail
   for (; n > Legalize(PageSize); n /= 2)
     try to grow mapping at the end by n bytes

For the second loop, you'd use mremap() on Linux, VirtualAlloc() passing the lpAddress first arg, and not even trying on OSX (b/c OSX is all 64-bit).

With this strategy, we never release memory, we just find the first biggest, rough allocation that succeeds and then grow it by increasingly small amounts.

WDYT?

@@ +503,3 @@
>  
> +    // This can't happen except via the shell toggling signals.enabled.
> +    if (buffer->isWasmMalloced()) {

IIUC, now this cannot happen at all: with createForWasm and prepareForAsmJS produce isWasmMapped().  In that case, can you delete one of the flags and rename the other to WASM and rename the query to isWasm()?

@@ +515,5 @@
> +    size_t mappedSize = wasm::LegalizeMemoryLength(buffer->byteLength());
> +    void* data = ArrayRawBufferObject::AllocateWasmMappedMemory(buffer->byteLength(), mappedSize);
> +    if (!data) {
> +        // Note - we don't need the same backoff search as in WASM, since we don't over-map to
> +        // allow growth in Asm.JS

"asm.js."

@@ +623,5 @@
> +    MOZ_ASSERT(isWasm());
> +    uint8_t* data = dataPointer();
> +
> +    if (delta == 0)
> +        return true;

nit: could you move above the decl of 'data', putting 'data' the line before 'curSize'?

@@ +632,5 @@
> +    MOZ_RELEASE_ASSERT(((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);
> +    MOZ_RELEASE_ASSERT(
> +            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);
> +    MOZ_RELEASE_ASSERT(
> +            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize <= (uint64_t) mappedSize());

This is kindof verbose for a preconditions-are-met check.  I think you could achieve roughly the same effect by MOZ_RELEASE_ASSERT()ing below that `newSize > curSize`.

@@ +693,5 @@
>              // The ABO is taking ownership, so account the bytes against the zone.
>              size_t nAllocated = nbytes;
>              if (contents.kind() == MAPPED)
>                  nAllocated = JS_ROUNDUP(nbytes, js::gc::SystemPageSize());
> +            else if (contents.kind() == WASM_MAPPED) {

nit: need { } around then branch since else branch has { }

@@ +695,5 @@
>              if (contents.kind() == MAPPED)
>                  nAllocated = JS_ROUNDUP(nbytes, js::gc::SystemPageSize());
> +            else if (contents.kind() == WASM_MAPPED) {
> +                ArrayRawBufferObject* rawBuf =
> +                    (ArrayRawBufferObject*)(contents.data() - sizeof(ArrayRawBufferObject));

This 'pointer - sizeof(ArrayRawBufferObject)' shows up quite a few times; I think you could factor out almost all of them if you added a method to ArrayBufferObject::BufferContents that returned the header* and also asserted isWasm().

::: js/src/vm/ArrayBufferObject.h
@@ +72,5 @@
>  
>  uint32_t AnyArrayBufferByteLength(const ArrayBufferObjectMaybeShared* buf);
> +uint32_t AnyArrayBufferActualByteLength(const ArrayBufferObjectMaybeShared* buf);
> +size_t AnyArrayBufferMappedSize(const ArrayBufferObjectMaybeShared* buf);
> +bool AnyArrayGrowForWasm(ArrayBufferObjectMaybeShared* buf, uint32_t delta);

All three of these added methods only apply to an ArrayBufferObject where isWasm()==true || is<SharedArrayBuffer>() (with this implicit assumption that a SAB of appropriate length is prepared appropriately).  Thus I think we should:
 - s/AnyArrayBuffer/WasmArrayBuffer/ for these three functions
 - either remove the methods from ArrayBufferObjectMaybeShared/ArrayBufferObject since they only apply to *some* buffers or add wasm prefix (growForWasm is fine as is)
 - rename ArrayRawBufferObject to WasmArrayRawBuffer (no "Object")

@@ +103,5 @@
>  typedef Rooted<ArrayBufferObjectMaybeShared*> RootedArrayBufferObjectMaybeShared;
>  typedef Handle<ArrayBufferObjectMaybeShared*> HandleArrayBufferObjectMaybeShared;
>  typedef MutableHandle<ArrayBufferObjectMaybeShared*> MutableHandleArrayBufferObjectMaybeShared;
>  
> +class ArrayRawBufferObject

Could you move this class to right before its method definitions in ArrayBufferObject.cpp?  I don't there are any uses of it in headers and it's sorta an impl detail of createForWasm/prepareForAsmJS.

@@ +116,5 @@
> +        MOZ_ASSERT(buffer == dataPointer());
> +    }
> +
> +  public:
> +    static ArrayRawBufferObject* New(uint8_t* p, uint32_t length, size_t mappedSize) {

This method only has one use in a private method so I'd just inline it and remove.

@@ +124,5 @@
> +        return obj;
> +    }
> +
> +    static void* AllocateWasmMappedMemory(uint32_t numBytes, size_t mappedSize);
> +    static void ReleaseWasmMappedMemory(void* mem);

I'd rename to just Allocate()/Release(); the class prefix says the rest.

@@ +140,5 @@
> +        return length_;
> +    }
> +
> +    void setByteLength(uint32_t newLen) {
> +        length_ = newLen;

Probably good to rename to actualByteLength()/setActualByteLength()/actualByteLength_ and add a comment that it's temporary; until proper wasm resizing is fully implemented.
Attachment #8781199 - Flags: review+ → review?(sunfish)
(Assignee)

Comment 27

2 years ago
Created attachment 8783260 [details] [diff] [review]
grow_mem_candidate_v32.diff

Tis the patch that keeps on giving :)!

Fixed Luke's comments from last round (+clean try run). Potential outstanding issues with the patch:

- ArrayBufferObjectMaybeShared::wasmSetMaxSize and ArrayBufferObjectMaybeShared::maxSize don't support SharedArrayBufferObjects, but are called on them in test. I don't know how to handle those right now?

- I don't see how we can avoid the double-comitting in prepareForAsmJS given the requirement that we have signals on all platforms. Unless we figure out how to do that, I should probably really remove WASM_MALLOCED.

Thank you all for the patience!

(In reply to Luke Wagner [:luke] from comment #26)
> Comment on attachment 8781199 [details] [diff] [review]
> grow_mem_candidate_v21.diff
> 
> Review of attachment 8781199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking very good, but one last close sweep found a few non-nit issues, so
> probably need one more pass:
> 
> ::: js/src/asmjs/WasmCode.cpp
> @@ +111,5 @@
> > +#ifndef WASM_HUGE_MEMORY
> > +    static_assert(sizeof(uint32_t) == sizeof(size_t), "Must be 32 bit if we don't have HUGE_MEMORY");
> > +    uint32_t length = memory->buffer().mappedSize();
> > +#else
> > +    // On x64 with 4G guard regions we still might get some BCs due to atomics.
> 
> nit: I'd spell out "bounds checks" in this comment.
> 

Fixed.

> @@ +114,5 @@
> > +#else
> > +    // On x64 with 4G guard regions we still might get some BCs due to atomics.
> > +    // For those check up to the maxMemoryLength. This should be inside the 4G
> > +    // guard region.
> > +    uint32_t length = metadata.maxMemoryLength.valueOr(metadata.minMemoryLength);
> 
> In theory, atomics don't support faulting *at all*.  Thus, in both the
> WASM_HUGE_MEMORY and not cases, using mappedSize or maxMemoryLength is
> invalid: it'll allow atomics to fault!  However, I'm thinking now that it's
> perfectly fine for atomics to fault: the original reason to disallow was
> because atomics started with the bad asm.js non-throwing semantics and this
> was hard to implement; but now they have throwing semantics so I think the
> signal handling machinery just works (apparently, assuming our tests are
> hitting this!).
> 
> So it seems like we should just remove all the isAtomic() special cases and
> stop pretending we're not faulting on atomic accesses.
> 
> With that, I think we can unconditionally say 'length =
> memory->buffer().mappedSize()'.  (Which also seems necessary since otherwise
> aren't we feeding in un-legalized length values?)  It'd be good to add a
> MOZ_RELEASE_ASSERT(length == LegalizeMemoryLength(length)).

Fixed.

> 
> ::: js/src/asmjs/WasmGenerator.h
> @@ +53,3 @@
> >      MemoryUsage               memoryUsage;
> >      mozilla::Atomic<uint32_t> minMemoryLength;
> > +    Maybe<uint32_t>  maxMemoryLength;
> 
> nit: can you align 'max' with 'min'?
> 

Fixed.

> ::: js/src/asmjs/WasmInstance.cpp
> @@ +296,5 @@
> > +        // The maxMemoryLength can be both greather than mappedSize (if we don't have enough
> > +        // address space) and smaller than (due to rounding/slop pages).
> > +        // So we need to check against the minimum of the two to be safe.
> > +        size_t maxGrowLength = Min(((size_t)*(metadata().maxMemoryLength)),
> > +                                   memory_->buffer().mappedSize());
> 
> So I think this isn't quite sufficient: many instances could be importing
> the same memory and we have to respect all of their maxes.  Thus, it's not
> sufficient to compare only against *this* module's static maxMemoryLength. 
> (I think the design docs need improvement here.)  Rather than attempting to
> maintain a linked list of linked instances, I think what we need to do is
> this: every time we instantiate a module against a memory (so in
> Module::instantiateMemory(), we take the Min() of the current max and the
> newly-instantiated module's maxMemoryLength.  Because legalization
> necessarily adds slop which we must maintain, I think this maxMemoryLength
> needs to be a separate field from mappedSize with the invariant
> buffer.maxMemoryLength <= buffer.mappedSize.  With that new field, you can
> simply check against buffer().maxMemoryLength() here.  To wit: we can unmap
> mapped memory if Legalize(buffer.maxMemoryLength) < buffer.mappedSize.
> 

Fixed.

> Also, nit: I think you can Min<size_t>() instead of casting the argument.
> 

Fixed.

> ::: js/src/asmjs/WasmJS.cpp
> @@ +762,5 @@
> >          return false;
> >      }
> >  
> >      uint32_t bytes = uint32_t(initialDbl) * PageSize;
> > +    size_t mappedSize = LegalizeMemoryLength(bytes);
> 
> Could you add a TODO here for using the 'maximum' property instead?  (or
> just add it :)

Fixed.

> 
> ::: js/src/asmjs/WasmModule.cpp
> @@ +518,5 @@
> >      if (memory) {
> >          buffer = &memory->buffer();
> >          uint32_t length = buffer->byteLength();
> > +
> > +        // It's not an error to import a memory, whose mapped size is less than
> 
> nit: remove ,
> 

Fixed.

> ::: js/src/asmjs/WasmSignalHandlers.cpp
> @@ +605,5 @@
> >  EmulateHeapAccess(EMULATOR_CONTEXT* context, uint8_t* pc, uint8_t* faultingAddress,
> >                    const MemoryAccess* memoryAccess, const Instance& instance)
> >  {
> >      MOZ_RELEASE_ASSERT(instance.codeSegment().containsFunctionPC(pc));
> > +    MOZ_RELEASE_ASSERT(wasm::HaveSignalHandlers());
> 
> Can remove since in signal handler :)
> 

Fixed.


> @@ +802,4 @@
> >      }
> >  
> > +    MOZ_RELEASE_ASSERT(instance->codeSegment().containsFunctionPC(pc));
> > +    MOZ_RELEASE_ASSERT(wasm::HaveSignalHandlers());
> 
> For the former assert, I think we should 'return false' if
> !containsFunctionPC.  That means we'll crash at the faulting insn which is a
> bit nicer when debugging.  For the latter, I'm not sure it adds much value
> since we're in the signal handler :)  Ditto for the OSX and Unix copies
> below.
> 

Fixed. Just removed the asserts since in all cases we have an earlier if guarding
containsFunctionPC(pc)

> @@ +1170,5 @@
> > +#else
> > +# if defined(JS_CODEGEN_X86)
> > +    MOZ_RELEASE_ASSERT(signal == Signal::SegFault);
> > +    *ppc = instance->codeSegment().outOfBoundsCode();
> > +# else // JS_CODEGEN_ARM
> 
> Since BusError is only installed on ARM, how about flipping the #else around
> so it's:
> 
> #if defined(WASM_HUGE_MEMORY)
> MOZ_RELEASE_ASSERT(signal == Signal::SegFault);
> ...
> #elif defined(JS_CODEGEN_ARM)
> MOZ_RELEASE_ASSERT(signal == Signal::BusError || signal == Signal::SegFault);
> ...
> #else
> MOZ_RELEASE_ASSERT(signal == Signal::SegFault);
> ...
> #endif
> 

Fixed.


> @@ +1172,5 @@
> > +    MOZ_RELEASE_ASSERT(signal == Signal::SegFault);
> > +    *ppc = instance->codeSegment().outOfBoundsCode();
> > +# else // JS_CODEGEN_ARM
> > +    MOZ_RELEASE_ASSERT(signal == Signal::BusError || signal == Signal::SegFault);
> > +
> 
> nit: can you remove \n for symmetry with above?
> 

Fixed.

> @@ +1282,5 @@
> >  }
> >  #endif
> >  
> > +static bool sTried = false;
> > +static bool sResult = false;
> 
> Sorry I wasn't more clear earlier, I just meant to rename these to
> sTriedInstallSignalHandlers/sHaveSignalHandlers, resp.
> 

Fixed.

> ::: js/src/jit/MacroAssembler.h
> @@ +1330,5 @@
> >  
> >      // WasmTableCallIndexReg must contain the index of the indirect call.
> >      void wasmCallIndirect(const wasm::CallSiteDesc& desc, const wasm::CalleeDesc& callee);
> >  
> > +    void wasmCallBuiltinInstanceMethod(const ABIArg& instanceArg,
> 
> For symmetry with preceding comments, probably good to be clear that this
> method implicitly preserves TLS and pinned regs.
> 

Fixed.


> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +383,1 @@
> >      MOZ_ASSERT(numBytes % wasm::PageSize == 0);
> 
> Can you also assert mappedSize % gc::SystemPageSize() == 0?

Fixed.

> 
> @@ +383,4 @@
> >      MOZ_ASSERT(numBytes % wasm::PageSize == 0);
> >  
> > +    uint64_t actualMappedSize = mappedSize + gc::SystemPageSize();
> > +    uint64_t actualNumBytes = numBytes + gc::SystemPageSize();
> 
> Unfortunately, now we have two meanings for "actual".  Maybe name to
> mappedSizeWithHeader/numBytesWithHeader?
> 

Fixed.

> @@ +452,5 @@
> > +        // If fail, apply binary search for the maximum allocatable chunk in the range
> > +        // [wasm::legalizeMappedSize(numBytes), mapSize-1]
> > +        size_t min = wasm::LegalizeMemoryLength(numBytes), max = mapSize;
> > +
> > +        MOZ_RELEASE_ASSERT(numBytes <= min && min <= max && mapSize <= max);
> 
> The last conjunct doesn't seem to add anything given preceding init.
> 

Fixed

> @@ +479,1 @@
> >          if (!data) {
> 
> I worry about the race condition where an allocation succeeds at size N, we
> release it, iterate, exit the loop, then try allocate it again, but now the
> allocation fails (due to race) and we throw.  I can see this happening if a
> site loads N workers, each with a large-ish max.  If we had Linux's mremap()
> everywhere, then we wouldn't have to release the memory; we could mremap()
> in-place.  Thinking through a couple of different variations, the best
> heuristic I can think of that avoids this race is:
> 
>    void* p = null
>    for (n = mappedSize; n >= minSize; n = clamp(legalize(n / 2), minSize))
>      p = allocate n
>      if (p) break;
>    if (!p) fail
>    for (; n > Legalize(PageSize); n /= 2)
>      try to grow mapping at the end by n bytes
> 
> For the second loop, you'd use mremap() on Linux, VirtualAlloc() passing the
> lpAddress first arg, and not even trying on OSX (b/c OSX is all 64-bit).
> 
> With this strategy, we never release memory, we just find the first biggest,
> rough allocation that succeeds and then grow it by increasingly small
> amounts.
> 
> WDYT?
> 

Fixed.

> @@ +503,3 @@
> >  
> > +    // This can't happen except via the shell toggling signals.enabled.
> > +    if (buffer->isWasmMalloced()) {
> 
> IIUC, now this cannot happen at all: with createForWasm and prepareForAsmJS
> produce isWasmMapped().  In that case, can you delete one of the flags and
> rename the other to WASM and rename the query to isWasm()?
> 

Fixed.

> @@ +515,5 @@
> > +    size_t mappedSize = wasm::LegalizeMemoryLength(buffer->byteLength());
> > +    void* data = ArrayRawBufferObject::AllocateWasmMappedMemory(buffer->byteLength(), mappedSize);
> > +    if (!data) {
> > +        // Note - we don't need the same backoff search as in WASM, since we don't over-map to
> > +        // allow growth in Asm.JS
> 
> "asm.js."
> 

Fixed.

> @@ +623,5 @@
> > +    MOZ_ASSERT(isWasm());
> > +    uint8_t* data = dataPointer();
> > +
> > +    if (delta == 0)
> > +        return true;
> 
> nit: could you move above the decl of 'data', putting 'data' the line before
> 'curSize'?

Fixed.

> 
> @@ +632,5 @@
> > +    MOZ_RELEASE_ASSERT(((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);
> > +    MOZ_RELEASE_ASSERT(
> > +            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize < (uint64_t) UINT32_MAX);
> > +    MOZ_RELEASE_ASSERT(
> > +            ((uint64_t)curSize) + ((uint64_t)delta) * wasm::PageSize <= (uint64_t) mappedSize());
> 
> This is kindof verbose for a preconditions-are-met check.  I think you could
> achieve roughly the same effect by MOZ_RELEASE_ASSERT()ing below that
> `newSize > curSize`.
> 

TODO: Convert to CheckedInt

> @@ +693,5 @@
> >              // The ABO is taking ownership, so account the bytes against the zone.
> >              size_t nAllocated = nbytes;
> >              if (contents.kind() == MAPPED)
> >                  nAllocated = JS_ROUNDUP(nbytes, js::gc::SystemPageSize());
> > +            else if (contents.kind() == WASM_MAPPED) {
> 
> nit: need { } around then branch since else branch has { }
> 

Fixed.

> @@ +695,5 @@
> >              if (contents.kind() == MAPPED)
> >                  nAllocated = JS_ROUNDUP(nbytes, js::gc::SystemPageSize());
> > +            else if (contents.kind() == WASM_MAPPED) {
> > +                ArrayRawBufferObject* rawBuf =
> > +                    (ArrayRawBufferObject*)(contents.data() - sizeof(ArrayRawBufferObject));
> 
> This 'pointer - sizeof(ArrayRawBufferObject)' shows up quite a few times; I
> think you could factor out almost all of them if you added a method to
> ArrayBufferObject::BufferContents that returned the header* and also
> asserted isWasm().
> 

Fixed.

> ::: js/src/vm/ArrayBufferObject.h
> @@ +72,5 @@
> >  
> >  uint32_t AnyArrayBufferByteLength(const ArrayBufferObjectMaybeShared* buf);
> > +uint32_t AnyArrayBufferActualByteLength(const ArrayBufferObjectMaybeShared* buf);
> > +size_t AnyArrayBufferMappedSize(const ArrayBufferObjectMaybeShared* buf);
> > +bool AnyArrayGrowForWasm(ArrayBufferObjectMaybeShared* buf, uint32_t delta);
> 
> All three of these added methods only apply to an ArrayBufferObject where
> isWasm()==true || is<SharedArrayBuffer>() (with this implicit assumption
> that a SAB of appropriate length is prepared appropriately).  Thus I think
> we should:
>  - s/AnyArrayBuffer/WasmArrayBuffer/ for these three functions
>  - either remove the methods from
> ArrayBufferObjectMaybeShared/ArrayBufferObject since they only apply to
> *some* buffers or add wasm prefix (growForWasm is fine as is)
>  - rename ArrayRawBufferObject to WasmArrayRawBuffer (no "Object")
> 

Fixed.

> @@ +103,5 @@
> >  typedef Rooted<ArrayBufferObjectMaybeShared*> RootedArrayBufferObjectMaybeShared;
> >  typedef Handle<ArrayBufferObjectMaybeShared*> HandleArrayBufferObjectMaybeShared;
> >  typedef MutableHandle<ArrayBufferObjectMaybeShared*> MutableHandleArrayBufferObjectMaybeShared;
> >  
> > +class ArrayRawBufferObject
> 
> Could you move this class to right before its method definitions in
> ArrayBufferObject.cpp?  I don't there are any uses of it in headers and it's
> sorta an impl detail of createForWasm/prepareForAsmJS.
> 

Fixed.

> @@ +116,5 @@
> > +        MOZ_ASSERT(buffer == dataPointer());
> > +    }
> > +
> > +  public:
> > +    static ArrayRawBufferObject* New(uint8_t* p, uint32_t length, size_t mappedSize) {
> 
> This method only has one use in a private method so I'd just inline it and
> remove.
> 

Fixed.

> @@ +124,5 @@
> > +        return obj;
> > +    }
> > +
> > +    static void* AllocateWasmMappedMemory(uint32_t numBytes, size_t mappedSize);
> > +    static void ReleaseWasmMappedMemory(void* mem);
> 
> I'd rename to just Allocate()/Release(); the class prefix says the rest.
> 

Fixed.

> @@ +140,5 @@
> > +        return length_;
> > +    }
> > +
> > +    void setByteLength(uint32_t newLen) {
> > +        length_ = newLen;
> 
> Probably good to rename to
> actualByteLength()/setActualByteLength()/actualByteLength_ and add a comment
> that it's temporary; until proper wasm resizing is fully implemented.

Fixed.
Attachment #8781199 - Attachment is obsolete: true
Attachment #8781280 - Attachment is obsolete: true
Attachment #8781199 - Flags: review?(sunfish)
Attachment #8781199 - Flags: review?(luke)
Attachment #8783260 - Flags: review?(sunfish)
Attachment #8783260 - Flags: review?(luke)

Comment 28

2 years ago
Comment on attachment 8783260 [details] [diff] [review]
grow_mem_candidate_v32.diff

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

Unfortunately, we cannot regress asm.js 32-bit memory use b/c we have strong data to show OOMs are already a problem and this will definitely make them worse: we must keep the current WASM_MALLOC'd behavior for prepareForAsmJS.  The key is this: asm.js AB byteLengths are necessarily legal and not resizable.  Therefore, by the invariants you've stated, the current = mapped = buf.byteLength so just like with SAB, you can add a special-case to WasmArrayBufferMappedSize and other methods for when isWasmMalloced().  To make it easier to read, we could rename to ASMJS_MALLOCED/isAsmJSMalloced(), if you want since now it's only for asm.js.

::: js/src/asmjs/WasmCode.cpp
@@ +108,5 @@
>  static void
>  SpecializeToMemory(CodeSegment& cs, const Metadata& metadata, HandleWasmMemoryObject memory)
>  {
> +    uint32_t length = memory->buffer().wasmMaxSize();
> +    MOZ_RELEASE_ASSERT(length == LegalizeMemoryLength(length));

The max-size only serves to bound memory growth and (as request in createForWasm) isn't legalized; we should still bounds check against the *mapped* size here since it is legalized.

::: js/src/asmjs/WasmIonCompile.cpp
@@ +967,5 @@
> +
> +            // If instanceArg_ is not initialized then instanceArg_.kind() != ABIArg::Stack
> +            if (call->instanceArg_.kind() == ABIArg::Stack)
> +                call->instanceArg_ = ABIArg(call->instanceArg_.offsetFromArgBase() +
> +                                            call->spIncrement_);

Need { } if can't fit on one line

::: js/src/asmjs/WasmJS.cpp
@@ +773,5 @@
>      }
>  
>      uint32_t bytes = uint32_t(initialDbl) * PageSize;
> +    // TODO: Add Memory.maximum. Use Memory.maximum * PageSize for maxSize,
> +    uint32_t maxSize = LegalizeMemoryLength(bytes);

As noted in createForWasm, I think you can remove LegalizeMemoryLength() here.

::: js/src/asmjs/WasmModule.cpp
@@ +533,1 @@
>              !buffer->as<ArrayBufferObject>().isWasmMapped())

According to the comment, this test could be turned into a MOZ_RELEASE_ASSERT().

@@ +540,3 @@
>      } else {
> +        uint32_t maxSize = LegalizeMemoryLength(
> +                metadata_->maxMemoryLength.valueOr(metadata_->minMemoryLength));

An unspecified max basically means a max of ∞, so UINT32_MAX.  Also, based on createForWasm comment, shouldn't Legalize here.

::: js/src/asmjs/WasmTypes.cpp
@@ +649,5 @@
> +    return length;
> +}
> +
> +uint32_t
> +wasm::LegalizeMemoryLength(uint32_t requestedSize)

With the changes suggested elsewhere, I don't think we need separate LegalizeMemoryLength/LegalizeMapLength anymore: only the mappedLength gets legalized.

::: js/src/jit/x64/CodeGenerator-x64.cpp
@@ +697,5 @@
>  
> +    // We cannot emulate atomic accesses currently.
> +    masm.append(AsmJSMemoryAccess(before, (mir->isAtomicAccess() ?
> +                                  wasm::MemoryAccess::Throw :
> +                                  wasm::MemoryAccess::CarryOn)));

Can you align `wasm::MemoryAccess::*` with the `m` in `mir->isAtomicAccess`?

::: js/src/vm/ArrayBufferObject.cpp
@@ +422,5 @@
> +
> +class js::WasmArrayRawBuffer
> +{
> +    uint32_t length_;
> +    uint32_t maxSize_;

For symmetry with above, perhaps name it currentMax_?

@@ +508,5 @@
> +            return false;
> +# elif defined(XP_DARWIN)
> +        // No mechanism for remapping on MaxOS. Luckily shouldn't need it here
> +        // as its all 64 bit.
> +        MOZ_CRASH("Shouldn't get called on MacOs");

It is technically *possible* to execute this (Mac still executes 32-bit binaries), so I think this should just be `return false`.

@@ +510,5 @@
> +        // No mechanism for remapping on MaxOS. Luckily shouldn't need it here
> +        // as its all 64 bit.
> +        MOZ_CRASH("Shouldn't get called on MacOs");
> +#else // Linux
> +        if (MAP_FAILED == mremap(dataPointer(), curMapped, newMapped, 0))

`mremap` can move the mapping if it isn't able to grow in-place.  POSIX doesn't have, iiuc, an equivalent of what VirtualAlloc() is doing here.  It's fine if the memory moves, of course, it just means it can't be neatly expressed as member function.

@@ +591,4 @@
>  #  endif
>  }
> +
> +WasmArrayRawBuffer* ArrayBufferObject::BufferContents::header() const {

{ on new line

@@ +601,3 @@
>  {
>      MOZ_ASSERT(numBytes % wasm::PageSize == 0);
> +    MOZ_ASSERT(wasm::LegalizeMemoryLength(maxSize) == maxSize);

I think maxSize shouldn't have to be legalized; only the mappedSize.  Otherwise, I think size could possibly get greater than a max-size that isn't a valid arm immediate.

@@ +668,5 @@
>          JS_ReportError(cx, "ArrayBuffer can't be used by asm.js");
>          return false;
>      }
>  
> +    uint32_t maxSize = wasm::LegalizeMemoryLength(buffer->byteLength());

asm.js byteLength is already legalized.
(Assignee)

Comment 29

2 years ago
Created attachment 8783807 [details] [diff] [review]
grow_mem_candidate_v34.diff

(In reply to Luke Wagner [:luke] from comment #28)
> Comment on attachment 8783260 [details] [diff] [review]
> grow_mem_candidate_v32.diff
> 
> Review of attachment 8783260 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unfortunately, we cannot regress asm.js 32-bit memory use b/c we have strong
> data to show OOMs are already a problem and this will definitely make them
> worse: we must keep the current WASM_MALLOC'd behavior for prepareForAsmJS. 
> The key is this: asm.js AB byteLengths are necessarily legal and not
> resizable.  Therefore, by the invariants you've stated, the current = mapped
> = buf.byteLength so just like with SAB, you can add a special-case to
> WasmArrayBufferMappedSize and other methods for when isWasmMalloced().  To
> make it easier to read, we could rename to ASMJS_MALLOCED/isAsmJSMalloced(),
> if you want since now it's only for asm.js.
> 

Fixed.

> ::: js/src/asmjs/WasmCode.cpp
> @@ +108,5 @@
> >  static void
> >  SpecializeToMemory(CodeSegment& cs, const Metadata& metadata, HandleWasmMemoryObject memory)
> >  {
> > +    uint32_t length = memory->buffer().wasmMaxSize();
> > +    MOZ_RELEASE_ASSERT(length == LegalizeMemoryLength(length));
> 
> The max-size only serves to bound memory growth and (as request in
> createForWasm) isn't legalized; we should still bounds check against the
> *mapped* size here since it is legalized.
> 

Fixed. I've added an explicit wasmBoundsCheckLimit(). Its currently set to mappedSize.

> ::: js/src/asmjs/WasmIonCompile.cpp
> @@ +967,5 @@
> > +
> > +            // If instanceArg_ is not initialized then instanceArg_.kind() != ABIArg::Stack
> > +            if (call->instanceArg_.kind() == ABIArg::Stack)
> > +                call->instanceArg_ = ABIArg(call->instanceArg_.offsetFromArgBase() +
> > +                                            call->spIncrement_);
> 
> Need { } if can't fit on one line
> 

Fixed.

> ::: js/src/asmjs/WasmJS.cpp
> @@ +773,5 @@
> >      }
> >  
> >      uint32_t bytes = uint32_t(initialDbl) * PageSize;
> > +    // TODO: Add Memory.maximum. Use Memory.maximum * PageSize for maxSize,
> > +    uint32_t maxSize = LegalizeMemoryLength(bytes);
> 
> As noted in createForWasm, I think you can remove LegalizeMemoryLength()
> here.
> 

Fixed.

> ::: js/src/asmjs/WasmModule.cpp
> @@ +533,1 @@
> >              !buffer->as<ArrayBufferObject>().isWasmMapped())
> 
> According to the comment, this test could be turned into a
> MOZ_RELEASE_ASSERT().
>

Fixed. NOTE: I had to use isWasm() instead of isWasmMapped() here as isWasmMapped() caused assertion failures in tests.

> @@ +540,3 @@
> >      } else {
> > +        uint32_t maxSize = LegalizeMemoryLength(
> > +                metadata_->maxMemoryLength.valueOr(metadata_->minMemoryLength));
> 
> An unspecified max basically means a max of ∞, so UINT32_MAX.  Also, based
> on createForWasm comment, shouldn't Legalize here.
> 

Fixed.


> ::: js/src/asmjs/WasmTypes.cpp
> @@ +649,5 @@
> > +    return length;
> > +}
> > +
> > +uint32_t
> > +wasm::LegalizeMemoryLength(uint32_t requestedSize)
> 
> With the changes suggested elsewhere, I don't think we need separate
> LegalizeMemoryLength/LegalizeMapLength anymore: only the mappedLength gets
> legalized.
> 

Fixed.

> ::: js/src/jit/x64/CodeGenerator-x64.cpp
> @@ +697,5 @@
> >  
> > +    // We cannot emulate atomic accesses currently.
> > +    masm.append(AsmJSMemoryAccess(before, (mir->isAtomicAccess() ?
> > +                                  wasm::MemoryAccess::Throw :
> > +                                  wasm::MemoryAccess::CarryOn)));
> 
> Can you align `wasm::MemoryAccess::*` with the `m` in `mir->isAtomicAccess`?
> 

Fixed.

> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +422,5 @@
> > +
> > +class js::WasmArrayRawBuffer
> > +{
> > +    uint32_t length_;
> > +    uint32_t maxSize_;
> 
> For symmetry with above, perhaps name it currentMax_?
> 

Fixed. Also renamed current in comment above to length for symmetry.

> @@ +508,5 @@
> > +            return false;
> > +# elif defined(XP_DARWIN)
> > +        // No mechanism for remapping on MaxOS. Luckily shouldn't need it here
> > +        // as its all 64 bit.
> > +        MOZ_CRASH("Shouldn't get called on MacOs");
> 
> It is technically *possible* to execute this (Mac still executes 32-bit
> binaries), so I think this should just be `return false`.

Fixed.

> 
> @@ +510,5 @@
> > +        // No mechanism for remapping on MaxOS. Luckily shouldn't need it here
> > +        // as its all 64 bit.
> > +        MOZ_CRASH("Shouldn't get called on MacOs");
> > +#else // Linux
> > +        if (MAP_FAILED == mremap(dataPointer(), curMapped, newMapped, 0))
> 
> `mremap` can move the mapping if it isn't able to grow in-place.  POSIX
> doesn't have, iiuc, an equivalent of what VirtualAlloc() is doing here. 
> It's fine if the memory moves, of course, it just means it can't be neatly
> expressed as member function.
> 

Fixed - no fix neccessary.

> @@ +591,4 @@
> >  #  endif
> >  }
> > +
> > +WasmArrayRawBuffer* ArrayBufferObject::BufferContents::header() const {
> 
> { on new line
> 

Fixed.

> @@ +601,3 @@
> >  {
> >      MOZ_ASSERT(numBytes % wasm::PageSize == 0);
> > +    MOZ_ASSERT(wasm::LegalizeMemoryLength(maxSize) == maxSize);
> 
> I think maxSize shouldn't have to be legalized; only the mappedSize. 
> Otherwise, I think size could possibly get greater than a max-size that
> isn't a valid arm immediate.
> 

Fixed.

> @@ +668,5 @@
> >          JS_ReportError(cx, "ArrayBuffer can't be used by asm.js");
> >          return false;
> >      }
> >  
> > +    uint32_t maxSize = wasm::LegalizeMemoryLength(buffer->byteLength());
> 
> asm.js byteLength is already legalized.

Fixed.
Attachment #8783260 - Attachment is obsolete: true
Attachment #8783260 - Flags: review?(sunfish)
Attachment #8783260 - Flags: review?(luke)
Attachment #8783807 - Flags: review?(sunfish)
Attachment #8783807 - Flags: review?(luke)
(Assignee)

Comment 30

2 years ago
Created attachment 8784219 [details] [diff] [review]
grow_mem_candidate_v35.diff

- Fixed importing a memory with max size bigger than the declared module to be an instantiation time error
- Propagate Maybe<uint32_t> for max size everywhere for memories
- Removed setMaxSize and related logic
- Added maximum to WebAssembly.Memory constructor
- Fixed import-export test
Attachment #8783807 - Attachment is obsolete: true
Attachment #8783807 - Flags: review?(sunfish)
Attachment #8783807 - Flags: review?(luke)
Attachment #8784219 - Flags: review?(sunfish)
Attachment #8784219 - Flags: review?(luke)

Comment 31

2 years ago
Comment on attachment 8784219 [details] [diff] [review]
grow_mem_candidate_v35.diff

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

Got to focus on ArrayBufferObject.cpp.  Almost done, looking very good, but I found a few non-nits that should be straightforward to fix.

::: js/src/asmjs/WasmJS.cpp
@@ +748,5 @@
>      }
>  
>      JSAtom* initialAtom = Atomize(cx, "initial", strlen("initial"));
> +    JSAtom* maximumAtom = Atomize(cx, "maximum", strlen("maximum"));
> +    if (!initialAtom || !maximumAtom)

Unfortunately you have to test after each fallible call b/c once an error is reported (and so an exception is pending) on a JSContext, you must propagate 'return false' immediately.

@@ +754,5 @@
>  
>      RootedObject obj(cx, &args[0].toObject());
> +    RootedId id(cx, AtomToId(initialAtom)),
> +             maxId(cx, AtomToId(maximumAtom));
> +    RootedValue initialVal(cx), maxVal(cx);

nit: s/id/initialId/.  Can you also move declarations of maxId and maxVal down to right before their first use (and maxDbl too).

@@ +771,5 @@
> +    bool found;
> +    Maybe<uint32_t> maxSize = Nothing();
> +
> +    if (HasProperty(cx, obj, maxId, &found) && found) {
> +        if (!GetProperty(cx, obj, obj, id, &maxVal))

I think you mean maxId here?  It'd be good to add a test where they are different to catch this case.

@@ +790,1 @@
>      uint32_t bytes = uint32_t(initialDbl) * PageSize;

For symmetry, could you rename bytes to initialSize?

@@ +790,2 @@
>      uint32_t bytes = uint32_t(initialDbl) * PageSize;
> +    // TODO: Add Memory.maximum. Use Memory.maximum * PageSize for maxSize,

You can remove the TODO now :)

::: js/src/asmjs/WasmModule.cpp
@@ +532,5 @@
> +        // For wasm we require that either both memory and module don't specify a max size
> +        // OR that the memory's max size is less than the modules.
> +        if (!metadata_->isAsmJS() &&
> +            (metadata_->maxMemoryLength.isSome() != buffer->wasmMaxSize().isSome() ||
> +             metadata_->maxMemoryLength < buffer->wasmMaxSize())) {

{ on new line

@@ +537,5 @@
>              JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_WASM_BAD_IMP_SIZE, "Memory");
>              return false;
>          }
>  
>          // This can't happen except via the shell toggling signals.enabled.

I think the comment is stale now

@@ +543,5 @@
> +                           buffer->as<ArrayBufferObject>().isWasm());
> +
> +        // We currently assume SharedArrayBuffer => asm.js. Can remove this
> +        // once wasmMaxSize/mappedSize/growForWasm have been implemented in SAB
> +        MOZ_RELEASE_ASSERT(!buffer->is<SharedArrayBufferObject>() || metadata_->isAsmJS());

In general, I think we only need MOZ_RELEASE_ASSERT() for corner cases that might only be caught in the field.  For stuff like this, MOZ_ASSERT() is fine b/c we'll catch this immediately in testing when we allow.  Also, you can MOZ_ASSERT_IF().

::: js/src/vm/ArrayBufferObject.cpp
@@ +384,5 @@
> + *
> + * The linear heap in Wasm is an mmaped array buffer. Several
> + * constants manage its lifetime:
> + *
> + *  - length - the current size of the buffer. Acesses in the range [0, current]

nit: "current size" is a bit ambiguous, how about "wasm-visible current length".  s/[0, current]/[0, length]/

@@ +387,5 @@
> + *
> + *  - length - the current size of the buffer. Acesses in the range [0, current]
> + *             succeed. May only increase
> + *
> + *  - bc_limit - size against which we perform bounds checks. It is always a

nit: perhaps better to spell out boundsCheckLimit for easier grepping

@@ +395,5 @@
> + *  - max - the optional declared limit on how much length can grow.
> + *
> + *  - mapped_size - the actual mmaped size. Access in the range
> + *             [0, mapped_size] will either succeed, or be handled by the
> + *             wasm signal handlers.

nit: indention here and above.  To avoid giving each bullet a separate start column, I'd just wrap back to the beginning

@@ +404,5 @@
> + *     COMMITED
> + *                \___________________________________________/
> + *                                    SLOP
> + * \___________________________________________________________/
> + *                         MAPPED

It'd be useful to include WasmArrayRawBuffer and its enclosing page in this diagram and then a little arrow showing what the ArrayBuffer pointes to

@@ +413,5 @@
> + *  - on ARM bc_limit must be a valid ARM immediate.
> + *  - if max is not specified, bc_limit/mapped_size may grow. They are
> + *    otherwise constant.
> + *
> + * NOTE: For asm.js on non-x64 we need to guarantee that

s/need to//

@@ +430,5 @@
> + *    grow memory up to Min(max, bc_limit) without having to patch/update
> + *    the bounds checks.
> + *
> + *  - from bc_limit to mapped_size - (Note: In current patch 0) - this part
> + *    of the patch allows us to bounds check against base pointers and fold

s/patch/SLOP/

@@ +431,5 @@
> + *    the bounds checks.
> + *
> + *  - from bc_limit to mapped_size - (Note: In current patch 0) - this part
> + *    of the patch allows us to bounds check against base pointers and fold
> + *    some constant offsets inside the loads. This enables better BCE.

nit: spell out Bounds Check Elimination

@@ +443,5 @@
> +    size_t mappedSize_;
> +
> +  protected:
> +    WasmArrayRawBuffer(uint8_t* buffer, uint32_t length, Maybe<uint32_t> maxSize, size_t mappedSize)
> +        : length_(length), maxSize_(maxSize), mappedSize_(mappedSize)

nit: two space indent of :

@@ +481,5 @@
> +    uint32_t boundsCheckLimit() const {
> +        return (uint32_t) Min<size_t>(mappedSize_, UINT32_MAX);
> +    }
> +
> +    bool growCommitted(uint32_t deltaBytes)

Symmetric to below suggested change, I'd rename to growLength(), take deltaLength and assert the delta is a multiple of wasm::PageSize.

@@ +490,5 @@
> +        MOZ_RELEASE_ASSERT(newSize.isValid() && newSize.value() <= maxSize().valueOr(UINT32_MAX));
> +
> +        // TODO: When we add realloc with patching, here attempt to grow the
> +        // mapped size.
> +        if (newSize.value() >= boundsCheckLimit())

If there is no max, then we won't be here at all.  Also instead I think we should bounds check newSize against maxSize since these are both the wasm semantic values and the bounds-check-limit is an internal legalized value.

@@ +493,5 @@
> +        // mapped size.
> +        if (newSize.value() >= boundsCheckLimit())
> +            return false;
> +
> +        uint8_t* dataEnd = dataPointer() + curSize.value();

Probably want to assert dataEnd is page-aligned.

@@ +508,5 @@
> +#  endif
> +
> +        MemProfiler::SampleNative(dataEnd, deltaBytes);
> +
> +        length_ = newSize.value();

This assignment makes me think that we're mixing "length" and "size" in our field names here.  Note that Metadata stores 'maxMemoryLength'.

@@ +514,5 @@
> +    }
> +
> +    // Try and grow the mapped region of memory. Does not changes current or
> +    // max size. Does not move memory if no space to grow.
> +    bool growReserved(uint32_t deltaBytes)

The return value isn't checked, so I'd return 'void' and prepend 'try' to the name.

Next, instead of passing deltaBytes (which aren't necessarily a valid increment for maxSize), I'd pass deltaMaxSize instead (asserting deltaMaxSize is a multiple of wasm::pageSize) and Legalize inside this function.

Given that change, I think a better name is tryGrowMaxSize() which is what we're really doing.  Also, with the belowmentioned change of not attempting to grow if !maxSize, you could assert maxSize is Some and take out the if (maxSize_) below.

@@ +529,5 @@
> +# elif defined(XP_DARWIN)
> +        // No mechanism for remapping on MaxOS. Luckily shouldn't need it here
> +        // as most MacOS configs are 64 bit
> +        return false;
> +#else // Linux

We actually run on a variety of Unices so I'd write "// Unix" instead.

@@ +548,4 @@
>  {
> +    MOZ_RELEASE_ASSERT(mappedSize <= SIZE_MAX - gc::SystemPageSize());
> +    MOZ_RELEASE_ASSERT(numBytes <= maxSize.valueOr(UINT32_MAX) && numBytes <= mappedSize);
> +    MOZ_RELEASE_ASSERT(mappedSize == wasm::LegalizeMapLength(mappedSize));

Since all the calls of Allocate() are already doing LegalizeMapLength(), how about removing the mappedSize parameter and having Allocate() do the LegalizeMapLength() itself.  This pairs well with the comment below in growForWasm (which suggests iterating over maxSize instead of mappedSize to make loop termination clear).  Grepping all uses of LegalizeMapLength() in the patch, that would put basically all of them inside WasmArrayRawBuffer so that mappedSize (and its legalization) was actually a detail of WasmArrayRawBuffer.

@@ +560,5 @@
>      if (!data)
>          return nullptr;
>  
> +    // Note we will waste a page on zero-sized memories here
> +    if (numBytesWithHeader &&

numBytesWithheader will always be > 0 (b/c gc::SystemPageSize), so you can remove this

@@ +572,5 @@
>      if (data == MAP_FAILED)
>          return nullptr;
>  
> +    // Note we will waste a page on zero-sized memories here
> +    if (numBytesWithHeader && mprotect(data, numBytesWithHeader, PROT_READ | PROT_WRITE)) {

ditto

@@ +587,5 @@
>  
> +    uint8_t* buffer = reinterpret_cast<uint8_t*>(data) + gc::SystemPageSize();
> +    uint8_t* base = buffer - sizeof(WasmArrayRawBuffer);
> +
> +    WasmArrayRawBuffer* rawBuf = new (base) WasmArrayRawBuffer(buffer, numBytes, maxSize,

We allow ourselves 'auto' when the RHS mentions the type of the LHS; that should make this a one-liner

@@ +613,4 @@
>  #  endif
>  }
> +
> +WasmArrayRawBuffer* ArrayBufferObject::BufferContents::header() const

nit: \n before 'ArrayBufferObject'

@@ +628,5 @@
>  
> +    // First try to map the maximum requested memory
> +    void* data = WasmArrayRawBuffer::Allocate(numBytes, maxSize, mapSize);
> +    if (!data) {
> +        // If fail, try to allocate a chunk with log backoff in the range

I think it'd simplify the below logic a bit if we had
  if (!maxSize) {
      ReportOutOfMemory(cx);
      return nullptr;
  }

@@ +632,5 @@
> +        // If fail, try to allocate a chunk with log backoff in the range
> +        // [wasm::legalizeMappedSize(numBytes), mapSize)
> +        size_t min = wasm::LegalizeMapLength(numBytes), cur = mapSize / 2;
> +
> +        MOZ_RELEASE_ASSERT(numBytes <= min);

I'd remove this assert since it's directly implied by the init above.

@@ +634,5 @@
> +        size_t min = wasm::LegalizeMapLength(numBytes), cur = mapSize / 2;
> +
> +        MOZ_RELEASE_ASSERT(numBytes <= min);
> +
> +        for (; cur > min; cur = wasm::LegalizeMapLength(cur / 2)) {

This loop condition feels vaguely hazardous of iloop: if you halve cur and then legalize, you might not make progress, treating Legalize() as a black box.  Rather it seems like you want to have your halving loop be iterating on the maxSize (asserting that it stays a multiple of wasm::PageSize) and then deriving mappedSize from that.

@@ +636,5 @@
> +        MOZ_RELEASE_ASSERT(numBytes <= min);
> +
> +        for (; cur > min; cur = wasm::LegalizeMapLength(cur / 2)) {
> +            data = WasmArrayRawBuffer::Allocate(numBytes,
> +                                                Some(Min<uint32_t>(maxSize.value(), cur)), cur);

It seems like maxSize can be None() here; but I think this Min() goes away with the above change.

@@ +654,5 @@
> +        for (size_t d = cur / 2; d >= wasm::PageSize; d /= 2) {
> +            size_t curMapped = buf->mappedSize();
> +            size_t deltaBytes = wasm::LegalizeMapLength(curMapped + d) - curMapped;
> +
> +            if (deltaBytes == 0)    break;

nit:
  if (deltaBytes == 0)
      break;

@@ +667,1 @@
>          return nullptr;

Also need to ReportOufOfMemory(cx);

@@ +723,5 @@
> +                       buffer->wasmActualByteLength() == buffer->wasmMaxSize().value());
> +    MOZ_RELEASE_ASSERT(buffer->wasmActualByteLength() == buffer->wasmBoundsCheckLimit());
> +    // Make sure bufferfer is page aligned, to make sure we can't accidentally
> +    // access memory we don't own.
> +    MOZ_RELEASE_ASSERT(buffer->wasmActualByteLength() % gc::SystemPageSize() == 0);

I'd remove this assert b/c the actual asm.js condition is IsValidAsmJSHeapLength() and that is checked by the caller so no real benefit to asserting here.

@@ +724,5 @@
> +    MOZ_RELEASE_ASSERT(buffer->wasmActualByteLength() == buffer->wasmBoundsCheckLimit());
> +    // Make sure bufferfer is page aligned, to make sure we can't accidentally
> +    // access memory we don't own.
> +    MOZ_RELEASE_ASSERT(buffer->wasmActualByteLength() % gc::SystemPageSize() == 0);
> +    MOZ_RELEASE_ASSERT((size_t)buffer->dataPointer() % gc::SystemPageSize() == 0);

This last assert probably doesn't hold (b/c it's malloced), and I don't think we depend on it (we're not mmaping anything), so I'd remove.

@@ +802,5 @@
> +{
> +    if (isWasmMapped())
> +        return contents().header()->mappedSize();
> +    else
> +        return wasmActualByteLength();

Perhaps just return byteLength() here (and below x2)

@@ +841,5 @@
> +
> +    // TODO: This is a hack just to get the SharedArrayBuffer tests passing for
> +    // now. Should be fixed when we add proper support for sharing/growing
> +    // memory with SAB
> +    MOZ_RELEASE_ASSERT(wasmMappedSize() <= UINT32_MAX);

I think asserting a uint32_t <= UINT32_MAX may give compiler warnings in some cases.  In general, I think we don't really need an assert here (or below).

@@ +846,5 @@
> +    return wasmMappedSize();
> +}
> +
> +Maybe<uint32_t>
> +ArrayBufferObjectMaybeShared::wasmMaxSize() const

From irc: it'd be good to remove the *MaybeShared methods that don't apply to SABs for now so we can see at the use sites the current dependency on an AB.

@@ +878,5 @@
> +        return true;
> +
> +    CheckedInt<uint32_t> curSize = wasmActualByteLength();
> +    CheckedInt<uint32_t> newSize = curSize + CheckedInt<uint32_t>(delta) * wasm ::PageSize;
> +    // Sanity. This should be guaranteed by Instance::growMemory

Can you put the comment before the first CheckedInt.  Also don't really need the "Sanity. " part.

@@ +922,2 @@
>              else if (contents.kind() == WASM_MAPPED)
> +                nAllocated = contents.header()->mappedSize() + gc::SystemPageSize();

It'd be nice to enapsulate this detail inside WasmArrayRawBuffer; could you add a allocatedBytes() member function that returns mappedSize() + gc::SystemPageSize?

::: js/src/vm/ArrayBufferObject.h
@@ +16,5 @@
>  #include "vm/Runtime.h"
>  #include "vm/SharedMem.h"
>  
>  typedef struct JSProperty JSProperty;
> +using mozilla::Maybe;

Generally, we only do the 'using mozilla::X' in .cpps.  An exception is WasmTypes.h, but that only injects the names into namespace wasm, not the global namespace.

@@ +217,5 @@
>          uint8_t* data() const { return data_; }
>          BufferKind kind() const { return kind_; }
>  
>          explicit operator bool() const { return data_ != nullptr; }
> +        WasmArrayRawBuffer *header() const;

This needs 'wasm' in the name somewhere; how about 'wasmBuffer'
(Assignee)

Comment 32

2 years ago
Created attachment 8784621 [details] [diff] [review]
grow_mem_candidate_v39.diff

Comments from last night addressed. Tested on Linux x86/x64. Try is closed right now :(, will test on rest as soon as it opens :)

Luke: given your comments on the namings of length/size, it might make sense to also rename maxSize to maxLength everywhere. Let me know what you think :)

(In reply to Luke Wagner [:luke] from comment #31)
> Comment on attachment 8784219 [details] [diff] [review]
> grow_mem_candidate_v35.diff
> 
> Review of attachment 8784219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Got to focus on ArrayBufferObject.cpp.  Almost done, looking very good, but
> I found a few non-nits that should be straightforward to fix.
> 
> ::: js/src/asmjs/WasmJS.cpp
> @@ +748,5 @@
> >      }
> >  
> >      JSAtom* initialAtom = Atomize(cx, "initial", strlen("initial"));
> > +    JSAtom* maximumAtom = Atomize(cx, "maximum", strlen("maximum"));
> > +    if (!initialAtom || !maximumAtom)
> 
> Unfortunately you have to test after each fallible call b/c once an error is
> reported (and so an exception is pending) on a JSContext, you must propagate
> 'return false' immediately.

Fixed.

> 
> @@ +754,5 @@
> >  
> >      RootedObject obj(cx, &args[0].toObject());
> > +    RootedId id(cx, AtomToId(initialAtom)),
> > +             maxId(cx, AtomToId(maximumAtom));
> > +    RootedValue initialVal(cx), maxVal(cx);
> 
> nit: s/id/initialId/.  Can you also move declarations of maxId and maxVal
> down to right before their first use (and maxDbl too).

Fixed.

> 
> @@ +771,5 @@
> > +    bool found;
> > +    Maybe<uint32_t> maxSize = Nothing();
> > +
> > +    if (HasProperty(cx, obj, maxId, &found) && found) {
> > +        if (!GetProperty(cx, obj, obj, id, &maxVal))
> 
> I think you mean maxId here?  It'd be good to add a test where they are
> different to catch this case.

Fixed.

> 
> @@ +790,1 @@
> >      uint32_t bytes = uint32_t(initialDbl) * PageSize;
> 
> For symmetry, could you rename bytes to initialSize?
> 

Fixed.

> @@ +790,2 @@
> >      uint32_t bytes = uint32_t(initialDbl) * PageSize;
> > +    // TODO: Add Memory.maximum. Use Memory.maximum * PageSize for maxSize,
> 
> You can remove the TODO now :)
> 

Fixed.

> ::: js/src/asmjs/WasmModule.cpp
> @@ +532,5 @@
> > +        // For wasm we require that either both memory and module don't specify a max size
> > +        // OR that the memory's max size is less than the modules.
> > +        if (!metadata_->isAsmJS() &&
> > +            (metadata_->maxMemoryLength.isSome() != buffer->wasmMaxSize().isSome() ||
> > +             metadata_->maxMemoryLength < buffer->wasmMaxSize())) {
> 
> { on new line

Fixed - no longer relevent - if broken up.

> 
> @@ +537,5 @@
> >              JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_WASM_BAD_IMP_SIZE, "Memory");
> >              return false;
> >          }
> >  
> >          // This can't happen except via the shell toggling signals.enabled.
> 
> I think the comment is stale now

Fixed.

> 
> @@ +543,5 @@
> > +                           buffer->as<ArrayBufferObject>().isWasm());
> > +
> > +        // We currently assume SharedArrayBuffer => asm.js. Can remove this
> > +        // once wasmMaxSize/mappedSize/growForWasm have been implemented in SAB
> > +        MOZ_RELEASE_ASSERT(!buffer->is<SharedArrayBufferObject>() || metadata_->isAsmJS());
> 
> In general, I think we only need MOZ_RELEASE_ASSERT() for corner cases that
> might only be caught in the field.  For stuff like this, MOZ_ASSERT() is
> fine b/c we'll catch this immediately in testing when we allow.  Also, you
> can MOZ_ASSERT_IF().

Fixed.

> 
> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +384,5 @@
> > + *
> > + * The linear heap in Wasm is an mmaped array buffer. Several
> > + * constants manage its lifetime:
> > + *
> > + *  - length - the current size of the buffer. Acesses in the range [0, current]
> 
> nit: "current size" is a bit ambiguous, how about "wasm-visible current
> length".  s/[0, current]/[0, length]/

Fixed.

> 
> @@ +387,5 @@
> > + *
> > + *  - length - the current size of the buffer. Acesses in the range [0, current]
> > + *             succeed. May only increase
> > + *
> > + *  - bc_limit - size against which we perform bounds checks. It is always a
> 
> nit: perhaps better to spell out boundsCheckLimit for easier grepping
> 

Fixed.

> @@ +395,5 @@
> > + *  - max - the optional declared limit on how much length can grow.
> > + *
> > + *  - mapped_size - the actual mmaped size. Access in the range
> > + *             [0, mapped_size] will either succeed, or be handled by the
> > + *             wasm signal handlers.
> 
> nit: indention here and above.  To avoid giving each bullet a separate start
> column, I'd just wrap back to the beginning

Fixed.

> 
> @@ +404,5 @@
> > + *     COMMITED
> > + *                \___________________________________________/
> > + *                                    SLOP
> > + * \___________________________________________________________/
> > + *                         MAPPED
> 
> It'd be useful to include WasmArrayRawBuffer and its enclosing page in this
> diagram and then a little arrow showing what the ArrayBuffer pointes to
> 

Fixed.

> @@ +413,5 @@
> > + *  - on ARM bc_limit must be a valid ARM immediate.
> > + *  - if max is not specified, bc_limit/mapped_size may grow. They are
> > + *    otherwise constant.
> > + *
> > + * NOTE: For asm.js on non-x64 we need to guarantee that
> 
> s/need to//
>

Fixed.
 
> @@ +430,5 @@
> > + *    grow memory up to Min(max, bc_limit) without having to patch/update
> > + *    the bounds checks.
> > + *
> > + *  - from bc_limit to mapped_size - (Note: In current patch 0) - this part
> > + *    of the patch allows us to bounds check against base pointers and fold
> 
> s/patch/SLOP/
> 

Fixed.

> @@ +431,5 @@
> > + *    the bounds checks.
> > + *
> > + *  - from bc_limit to mapped_size - (Note: In current patch 0) - this part
> > + *    of the patch allows us to bounds check against base pointers and fold
> > + *    some constant offsets inside the loads. This enables better BCE.
> 
> nit: spell out Bounds Check Elimination
> 

Fixed.

> @@ +443,5 @@
> > +    size_t mappedSize_;
> > +
> > +  protected:
> > +    WasmArrayRawBuffer(uint8_t* buffer, uint32_t length, Maybe<uint32_t> maxSize, size_t mappedSize)
> > +        : length_(length), maxSize_(maxSize), mappedSize_(mappedSize)
> 
> nit: two space indent of :
>

Fixed.
 
> @@ +481,5 @@
> > +    uint32_t boundsCheckLimit() const {
> > +        return (uint32_t) Min<size_t>(mappedSize_, UINT32_MAX);
> > +    }
> > +
> > +    bool growCommitted(uint32_t deltaBytes)
> 
> Symmetric to below suggested change, I'd rename to growLength(), take
> deltaLength and assert the delta is a multiple of wasm::PageSize.
> 

Fixed.

> @@ +490,5 @@
> > +        MOZ_RELEASE_ASSERT(newSize.isValid() && newSize.value() <= maxSize().valueOr(UINT32_MAX));
> > +
> > +        // TODO: When we add realloc with patching, here attempt to grow the
> > +        // mapped size.
> > +        if (newSize.value() >= boundsCheckLimit())
> 
> If there is no max, then we won't be here at all.  Also instead I think we
> should bounds check newSize against maxSize since these are both the wasm
> semantic values and the bounds-check-limit is an internal legalized value.
> 

Fixed.

> @@ +493,5 @@
> > +        // mapped size.
> > +        if (newSize.value() >= boundsCheckLimit())
> > +            return false;
> > +
> > +        uint8_t* dataEnd = dataPointer() + curSize.value();
> 
> Probably want to assert dataEnd is page-aligned.
> 

Fixed.

> @@ +508,5 @@
> > +#  endif
> > +
> > +        MemProfiler::SampleNative(dataEnd, deltaBytes);
> > +
> > +        length_ = newSize.value();
> 
> This assignment makes me think that we're mixing "length" and "size" in our
> field names here.  Note that Metadata stores 'maxMemoryLength'.
> 

Fixed. Renamed all to length.

> @@ +514,5 @@
> > +    }
> > +
> > +    // Try and grow the mapped region of memory. Does not changes current or
> > +    // max size. Does not move memory if no space to grow.
> > +    bool growReserved(uint32_t deltaBytes)
> 
> The return value isn't checked, so I'd return 'void' and prepend 'try' to
> the name.

Fixed.

> 
> Next, instead of passing deltaBytes (which aren't necessarily a valid
> increment for maxSize), I'd pass deltaMaxSize instead (asserting
> deltaMaxSize is a multiple of wasm::pageSize) and Legalize inside this
> function.
> 

Fixed.

> Given that change, I think a better name is tryGrowMaxSize() which is what
> we're really doing.  Also, with the belowmentioned change of not attempting
> to grow if !maxSize, you could assert maxSize is Some and take out the if
> (maxSize_) below.
> 

Fixed.

> @@ +529,5 @@
> > +# elif defined(XP_DARWIN)
> > +        // No mechanism for remapping on MaxOS. Luckily shouldn't need it here
> > +        // as most MacOS configs are 64 bit
> > +        return false;
> > +#else // Linux
> 
> We actually run on a variety of Unices so I'd write "// Unix" instead.
> 

Fixed.

> @@ +548,4 @@
> >  {
> > +    MOZ_RELEASE_ASSERT(mappedSize <= SIZE_MAX - gc::SystemPageSize());
> > +    MOZ_RELEASE_ASSERT(numBytes <= maxSize.valueOr(UINT32_MAX) && numBytes <= mappedSize);
> > +    MOZ_RELEASE_ASSERT(mappedSize == wasm::LegalizeMapLength(mappedSize));
> 
> Since all the calls of Allocate() are already doing LegalizeMapLength(), how
> about removing the mappedSize parameter and having Allocate() do the
> LegalizeMapLength() itself.  This pairs well with the comment below in
> growForWasm (which suggests iterating over maxSize instead of mappedSize to
> make loop termination clear).  Grepping all uses of LegalizeMapLength() in
> the patch, that would put basically all of them inside WasmArrayRawBuffer so
> that mappedSize (and its legalization) was actually a detail of
> WasmArrayRawBuffer.
> 

Fixed.

> @@ +560,5 @@
> >      if (!data)
> >          return nullptr;
> >  
> > +    // Note we will waste a page on zero-sized memories here
> > +    if (numBytesWithHeader &&
> 
> numBytesWithheader will always be > 0 (b/c gc::SystemPageSize), so you can
> remove this
> 

Fixed.

> @@ +572,5 @@
> >      if (data == MAP_FAILED)
> >          return nullptr;
> >  
> > +    // Note we will waste a page on zero-sized memories here
> > +    if (numBytesWithHeader && mprotect(data, numBytesWithHeader, PROT_READ | PROT_WRITE)) {
> 
> ditto
> 

Fixed.

> @@ +587,5 @@
> >  
> > +    uint8_t* buffer = reinterpret_cast<uint8_t*>(data) + gc::SystemPageSize();
> > +    uint8_t* base = buffer - sizeof(WasmArrayRawBuffer);
> > +
> > +    WasmArrayRawBuffer* rawBuf = new (base) WasmArrayRawBuffer(buffer, numBytes, maxSize,
> 
> We allow ourselves 'auto' when the RHS mentions the type of the LHS; that
> should make this a one-liner
> 

Fixed.

> @@ +613,4 @@
> >  #  endif
> >  }
> > +
> > +WasmArrayRawBuffer* ArrayBufferObject::BufferContents::header() const
> 
> nit: \n before 'ArrayBufferObject'
> 

Fixed.

> @@ +628,5 @@
> >  
> > +    // First try to map the maximum requested memory
> > +    void* data = WasmArrayRawBuffer::Allocate(numBytes, maxSize, mapSize);
> > +    if (!data) {
> > +        // If fail, try to allocate a chunk with log backoff in the range
> 
> I think it'd simplify the below logic a bit if we had
>   if (!maxSize) {
>       ReportOutOfMemory(cx);
>       return nullptr;
>   }
> 

Fixed.
Revisit: Stale comment.

> @@ +632,5 @@
> > +        // If fail, try to allocate a chunk with log backoff in the range
> > +        // [wasm::legalizeMappedSize(numBytes), mapSize)
> > +        size_t min = wasm::LegalizeMapLength(numBytes), cur = mapSize / 2;
> > +
> > +        MOZ_RELEASE_ASSERT(numBytes <= min);
> 
> I'd remove this assert since it's directly implied by the init above.
> 

Fixed.

> @@ +634,5 @@
> > +        size_t min = wasm::LegalizeMapLength(numBytes), cur = mapSize / 2;
> > +
> > +        MOZ_RELEASE_ASSERT(numBytes <= min);
> > +
> > +        for (; cur > min; cur = wasm::LegalizeMapLength(cur / 2)) {
> 
> This loop condition feels vaguely hazardous of iloop: if you halve cur and
> then legalize, you might not make progress, treating Legalize() as a black
> box.  Rather it seems like you want to have your halving loop be iterating
> on the maxSize (asserting that it stays a multiple of wasm::PageSize) and
> then deriving mappedSize from that.
> 

Fixed.

> @@ +636,5 @@
> > +        MOZ_RELEASE_ASSERT(numBytes <= min);
> > +
> > +        for (; cur > min; cur = wasm::LegalizeMapLength(cur / 2)) {
> > +            data = WasmArrayRawBuffer::Allocate(numBytes,
> > +                                                Some(Min<uint32_t>(maxSize.value(), cur)), cur);
> 
> It seems like maxSize can be None() here; but I think this Min() goes away
> with the above change.
> 

Fixed.

> @@ +654,5 @@
> > +        for (size_t d = cur / 2; d >= wasm::PageSize; d /= 2) {
> > +            size_t curMapped = buf->mappedSize();
> > +            size_t deltaBytes = wasm::LegalizeMapLength(curMapped + d) - curMapped;
> > +
> > +            if (deltaBytes == 0)    break;
> 
> nit:
>   if (deltaBytes == 0)
>       break;
> 

Fixed - removed.

> @@ +667,1 @@
> >          return nullptr;
> 
> Also need to ReportOufOfMemory(cx);

Fixed.

> 
> @@ +723,5 @@
> > +                       buffer->wasmActualByteLength() == buffer->wasmMaxSize().value());
> > +    MOZ_RELEASE_ASSERT(buffer->wasmActualByteLength() == buffer->wasmBoundsCheckLimit());
> > +    // Make sure bufferfer is page aligned, to make sure we can't accidentally
> > +    // access memory we don't own.
> > +    MOZ_RELEASE_ASSERT(buffer->wasmActualByteLength() % gc::SystemPageSize() == 0);
> 
> I'd remove this assert b/c the actual asm.js condition is
> IsValidAsmJSHeapLength() and that is checked by the caller so no real
> benefit to asserting here.
> 

Fixed.

> @@ +724,5 @@
> > +    MOZ_RELEASE_ASSERT(buffer->wasmActualByteLength() == buffer->wasmBoundsCheckLimit());
> > +    // Make sure bufferfer is page aligned, to make sure we can't accidentally
> > +    // access memory we don't own.
> > +    MOZ_RELEASE_ASSERT(buffer->wasmActualByteLength() % gc::SystemPageSize() == 0);
> > +    MOZ_RELEASE_ASSERT((size_t)buffer->dataPointer() % gc::SystemPageSize() == 0);
> 
> This last assert probably doesn't hold (b/c it's malloced), and I don't
> think we depend on it (we're not mmaping anything), so I'd remove.
> 

Fixed.

> @@ +802,5 @@
> > +{
> > +    if (isWasmMapped())
> > +        return contents().header()->mappedSize();
> > +    else
> > +        return wasmActualByteLength();
> 
> Perhaps just return byteLength() here (and below x2)
> 

Fixed.

> @@ +841,5 @@
> > +
> > +    // TODO: This is a hack just to get the SharedArrayBuffer tests passing for
> > +    // now. Should be fixed when we add proper support for sharing/growing
> > +    // memory with SAB
> > +    MOZ_RELEASE_ASSERT(wasmMappedSize() <= UINT32_MAX);
> 
> I think asserting a uint32_t <= UINT32_MAX may give compiler warnings in
> some cases.  In general, I think we don't really need an assert here (or
> below).
> 

Fixed.

> @@ +846,5 @@
> > +    return wasmMappedSize();
> > +}
> > +
> > +Maybe<uint32_t>
> > +ArrayBufferObjectMaybeShared::wasmMaxSize() const
> 
> From irc: it'd be good to remove the *MaybeShared methods that don't apply
> to SABs for now so we can see at the use sites the current dependency on an
> AB.
> 

Fixed.

> @@ +878,5 @@
> > +        return true;
> > +
> > +    CheckedInt<uint32_t> curSize = wasmActualByteLength();
> > +    CheckedInt<uint32_t> newSize = curSize + CheckedInt<uint32_t>(delta) * wasm ::PageSize;
> > +    // Sanity. This should be guaranteed by Instance::growMemory
> 
> Can you put the comment before the first CheckedInt.  Also don't really need
> the "Sanity. " part.
> 

Fixed.

> @@ +922,2 @@
> >              else if (contents.kind() == WASM_MAPPED)
> > +                nAllocated = contents.header()->mappedSize() + gc::SystemPageSize();
> 
> It'd be nice to enapsulate this detail inside WasmArrayRawBuffer; could you
> add a allocatedBytes() member function that returns mappedSize() +
> gc::SystemPageSize?
> 

Fixed.

> ::: js/src/vm/ArrayBufferObject.h
> @@ +16,5 @@
> >  #include "vm/Runtime.h"
> >  #include "vm/SharedMem.h"
> >  
> >  typedef struct JSProperty JSProperty;
> > +using mozilla::Maybe;
> 
> Generally, we only do the 'using mozilla::X' in .cpps.  An exception is
> WasmTypes.h, but that only injects the names into namespace wasm, not the
> global namespace.

Fixed.

> 
> @@ +217,5 @@
> >          uint8_t* data() const { return data_; }
> >          BufferKind kind() const { return kind_; }
> >  
> >          explicit operator bool() const { return data_ != nullptr; }
> > +        WasmArrayRawBuffer *header() const;
> 
> This needs 'wasm' in the name somewhere; how about 'wasmBuffer'

Fixed.
Attachment #8784219 - Attachment is obsolete: true
Attachment #8784219 - Flags: review?(sunfish)
Attachment #8784219 - Flags: review?(luke)
Attachment #8784621 - Flags: review?(sunfish)
Attachment #8784621 - Flags: review?(luke)

Comment 33

2 years ago
Comment on attachment 8784621 [details] [diff] [review]
grow_mem_candidate_v39.diff

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

\o/  Good work!  Just some trivial changes:

::: js/src/asmjs/WasmJS.cpp
@@ +752,5 @@
>          return false;
>  
> +    JSAtom* maximumAtom = Atomize(cx, "maximum", strlen("maximum"));
> +
> +    if (!maximumAtom)

nit: rm \n before test

@@ +771,5 @@
>          return false;
>      }
>  
> +    bool found;
> +    Maybe<uint32_t> maxSize = Nothing();

nit: I don't think you need to explicitly initialize to Nothing()

@@ +777,5 @@
> +
> +    if (HasProperty(cx, obj, maxId, &found) && found) {
> +        RootedValue maxVal(cx);
> +
> +        if (!GetProperty(cx, obj, obj, maxId, &maxVal))

nit: no \n between RootedValue and GetProperty

::: js/src/asmjs/WasmModule.cpp
@@ +532,5 @@
> +        // For wasm we require that either both memory and module don't specify a max size
> +        // OR that the memory's max size is less than the modules.
> +        if (!metadata_->isAsmJS()) {
> +            Maybe<uint32_t> memMaxSize =
> +                ((ArrayBufferObjectMaybeShared*)buffer)->as<ArrayBufferObject>().wasmMaxSize();

You shouldn't need the cast on 'buffer', just buffer->as<ArrayBufferObject>() should work (see below code).

::: js/src/vm/ArrayBufferObject.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "vm/ArrayBufferObject-inl.h"

SM #include ordering (https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.23include_ordering) should put this lower down, right before NativeObject-inl.h.  'make check-style' will report these violations (it is run as part of CI builds; I'd make sure to do a full try build before pushing :)

@@ +398,5 @@
> + *  [0, mapped_size] will either succeed, or be handled by the wasm signal
> + *  handlers.
> + *
> + * The below diagram shows the layout of the wams heap. The wasm-visible
> + * portion of the heap starts at 0. In reality, there is one extra page

nit: I'd leave out "In reality, "

@@ +408,5 @@
> + *       \  /
> + *        \ |
> + *  ________|____________________________________________________________
> + * |______|_|______________|___________________|____________|____________|
> + *-         0          length                 max  boundsCheckLimit  mapped_size

nit: s/max/maxSize/ s/mapped_size/mappedSize/ to match field names (here and below)

@@ +494,5 @@
> +        return mappedSize_ + gc::SystemPageSize();
> +    }
> +
> +    uint32_t boundsCheckLimit() const {
> +        return (uint32_t) Min<size_t>(mappedSize_, UINT32_MAX);

This is fishy and I think it's only valid because mappedSize_ is only > UINT32_MAX for WASM_HUGE_MEMORY and that's only the case for x64 where any immediate is legal.  But I think it's fine for now...

@@ +497,5 @@
> +    uint32_t boundsCheckLimit() const {
> +        return (uint32_t) Min<size_t>(mappedSize_, UINT32_MAX);
> +    }
> +
> +    bool growLength(uint32_t deltaLength)

MOZ_MUST_USE

@@ +499,5 @@
> +    }
> +
> +    bool growLength(uint32_t deltaLength)
> +    {
> +        MOZ_ASSERT(maxSize_.isSome() && deltaLength % wasm::PageSize == 0);

nit: we generally try to have 1 conjunct per assert, so I'd split this into two MOZ_ASSERT()s (since they're separate invariants) and put a \n after

@@ +505,5 @@
> +        CheckedInt<uint32_t> curLength = actualByteLength();
> +        CheckedInt<uint32_t> newLength = curLength + deltaLength;
> +        MOZ_RELEASE_ASSERT(newLength.isValid());
> +        // Guaranteed by Instance::growMemory
> +        MOZ_ASSERT(newLength.value() <= maxSize_.value());

nit: There are two "Guaranteed by Instance::growMemory" comments; I'd have one, right before the first assert, with a \n before to separate from the CheckedInt<>s

@@ +536,5 @@
> +        MOZ_RELEASE_ASSERT(deltaMaxSize % wasm::PageSize  == 0);
> +
> +        CheckedInt<uint32_t> curMax = maxSize_.value();
> +        CheckedInt<uint32_t> newMax = curMax + deltaMaxSize;
> +        MOZ_RELEASE_ASSERT(newMax.isValid());

Another nice release assert would be newMax.value() % wasm::PageSize == 0.

@@ +538,5 @@
> +        CheckedInt<uint32_t> curMax = maxSize_.value();
> +        CheckedInt<uint32_t> newMax = curMax + deltaMaxSize;
> +        MOZ_RELEASE_ASSERT(newMax.isValid());
> +
> +        size_t legalNewMapped = wasm::LegalizeMapLength(newMax.value());

nit: there's not an un-legal newMapped, so I'd go with newMappedSize to make the destination clear.

@@ +565,5 @@
>  {
> +    size_t mappedSize = wasm::LegalizeMapLength(maxSize.valueOr(numBytes));
> +
> +    MOZ_RELEASE_ASSERT(mappedSize <= SIZE_MAX - gc::SystemPageSize());
> +    MOZ_RELEASE_ASSERT(numBytes <= maxSize.valueOr(UINT32_MAX) && numBytes <= mappedSize);

I think this second assert is now fairly trivially implied by above.

@@ +570,2 @@
>      MOZ_ASSERT(numBytes % wasm::PageSize == 0);
> +    MOZ_ASSERT(mappedSize % wasm::PageSize == 0);

nit: I'd use gc::SystemPageSize() instead since that's the relevant property for mapping memory. Also technically I don't think legalize has to wasm::PageSize-align (e.g., if it uses a <64k guard page).

@@ +578,4 @@
>      if (!data)
>          return nullptr;
>  
> +    // Note we will waste a page on zero-sized memories here

nit: we're allocating a whole page, true, but that's just evident in the init of numBytesWithHeader, so I'd remove the comment (here and below).

@@ +603,4 @@
>  #  endif
>  
> +    uint8_t* buffer = reinterpret_cast<uint8_t*>(data) + gc::SystemPageSize();
> +    uint8_t* base = buffer - sizeof(WasmArrayRawBuffer);

nit: s/base/header/ then s/buffer/base/ to match field names in WasmArrayRawBuffer.

@@ +614,2 @@
>  {
> +    WasmArrayRawBuffer* rawBuf = (WasmArrayRawBuffer*)((uint8_t*)mem - sizeof(WasmArrayRawBuffer));

nit: s/rawBuf/header/

@@ +645,5 @@
> +    // First try to map the maximum requested memory
> +    void* data = WasmArrayRawBuffer::Allocate(numBytes, maxSize);
> +    if (!data) {
> +#ifdef  WASM_HUGE_MEMORY
> +        return nullptr;

ReportOutOfMemory(cx)

@@ +659,5 @@
> +
> +        for (; cur > numBytes; cur = cur / 2) {
> +            data = WasmArrayRawBuffer::Allocate(numBytes, Some(cur));
> +
> +            if (data)

nit: rm \n before test

@@ +668,5 @@
>              ReportOutOfMemory(cx);
>              return nullptr;
>          }
>  
> +        WasmArrayRawBuffer *buf  = (WasmArrayRawBuffer*) data - sizeof(WasmArrayRawBuffer);

Can you have WasmArrayRawBufer::Allocate() return a WasmArrayRawBuffer* instead (and then the callers can call ->dataPointer() as needed

@@ +679,5 @@
>  
> +    BufferContents contents = BufferContents::create<WASM_MAPPED>(data);
> +    ArrayBufferObject* buffer = ArrayBufferObject::create(cx, numBytes, contents);
> +
> +    if (!buffer) {

nit: rm \n before test

@@ +816,5 @@
> +        return contents().wasmBuffer()->mappedSize();
> +    else
> +        // Can use byteLength() instead of actualByteLength since if !wasmMapped()
> +        // then this is an asm.js buffer, and thus cannot grow.
> +        return byteLength();

nit: { } around then and else branches b/c multiline

@@ +855,5 @@
> +
> +    // TODO: This is a hack just to get the SharedArrayBuffer tests passing for
> +    // now. Should be fixed when we add proper support for sharing/growing
> +    // memory with SAB
> +    return wasmMappedSize();

nit: "hack" is a bit strong, how about: "TODO: when SharedArrayBuffer can be used from wasm, this should be replaced by SharedArrayBufferObject::wasmBoundsCheckLimit()"

::: js/src/vm/ArrayBufferObject.h
@@ +145,5 @@
>      };
>  
>      enum BufferKind {
>          PLAIN               = 0, // malloced or inline data
> +        ASMJS_MALLOCED       = 1,

nit: indent of =

@@ +213,5 @@
>          uint8_t* data() const { return data_; }
>          BufferKind kind() const { return kind_; }
>  
>          explicit operator bool() const { return data_ != nullptr; }
> +        WasmArrayRawBuffer *wasmBuffer() const;

nit: WasmArrayRawBuffer*
Attachment #8784621 - Flags: review?(luke) → review+
(Assignee)

Comment 34

2 years ago
Created attachment 8784998 [details] [diff] [review]
grow_mem_candidate_v40.diff

Addressed Lukes comments below. Also added rounding up to wasm::PageSize in the backoff calculations in createForWasm(). (was causing an assertion failure on windows)

(In reply to Luke Wagner [:luke] from comment #33)
> Comment on attachment 8784621 [details] [diff] [review]
> grow_mem_candidate_v39.diff
> 
> Review of attachment 8784621 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> \o/  Good work!  Just some trivial changes:
> 
> ::: js/src/asmjs/WasmJS.cpp
> @@ +752,5 @@
> >          return false;
> >  
> > +    JSAtom* maximumAtom = Atomize(cx, "maximum", strlen("maximum"));
> > +
> > +    if (!maximumAtom)
> 
> nit: rm \n before test
> 

Fixed.

> @@ +771,5 @@
> >          return false;
> >      }
> >  
> > +    bool found;
> > +    Maybe<uint32_t> maxSize = Nothing();
> 
> nit: I don't think you need to explicitly initialize to Nothing()
> 

Fixed.

> @@ +777,5 @@
> > +
> > +    if (HasProperty(cx, obj, maxId, &found) && found) {
> > +        RootedValue maxVal(cx);
> > +
> > +        if (!GetProperty(cx, obj, obj, maxId, &maxVal))
> 
> nit: no \n between RootedValue and GetProperty
> 

Fixed.

> ::: js/src/asmjs/WasmModule.cpp
> @@ +532,5 @@
> > +        // For wasm we require that either both memory and module don't specify a max size
> > +        // OR that the memory's max size is less than the modules.
> > +        if (!metadata_->isAsmJS()) {
> > +            Maybe<uint32_t> memMaxSize =
> > +                ((ArrayBufferObjectMaybeShared*)buffer)->as<ArrayBufferObject>().wasmMaxSize();
> 
> You shouldn't need the cast on 'buffer', just
> buffer->as<ArrayBufferObject>() should work (see below code).
> 

Fixed.

> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +3,5 @@
> >   * This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +#include "vm/ArrayBufferObject-inl.h"
> 
> SM #include ordering
> (https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.
> 23include_ordering) should put this lower down, right before
> NativeObject-inl.h.  'make check-style' will report these violations (it is
> run as part of CI builds; I'd make sure to do a full try build before
> pushing :)
> 

Not a bug - make check-style accepts this.

> @@ +398,5 @@
> > + *  [0, mapped_size] will either succeed, or be handled by the wasm signal
> > + *  handlers.
> > + *
> > + * The below diagram shows the layout of the wams heap. The wasm-visible
> > + * portion of the heap starts at 0. In reality, there is one extra page
> 
> nit: I'd leave out "In reality, "
> 

Fixed.

> @@ +408,5 @@
> > + *       \  /
> > + *        \ |
> > + *  ________|____________________________________________________________
> > + * |______|_|______________|___________________|____________|____________|
> > + *-         0          length                 max  boundsCheckLimit  mapped_size
> 
> nit: s/max/maxSize/ s/mapped_size/mappedSize/ to match field names (here and
> below)
> 

Fixed.

> @@ +494,5 @@
> > +        return mappedSize_ + gc::SystemPageSize();
> > +    }
> > +
> > +    uint32_t boundsCheckLimit() const {
> > +        return (uint32_t) Min<size_t>(mappedSize_, UINT32_MAX);
> 
> This is fishy and I think it's only valid because mappedSize_ is only >
> UINT32_MAX for WASM_HUGE_MEMORY and that's only the case for x64 where any
> immediate is legal.  But I think it's fine for now...
> 

Fixed.

> @@ +497,5 @@
> > +    uint32_t boundsCheckLimit() const {
> > +        return (uint32_t) Min<size_t>(mappedSize_, UINT32_MAX);
> > +    }
> > +
> > +    bool growLength(uint32_t deltaLength)
> 
> MOZ_MUST_USE
> 

Fixed.

> @@ +499,5 @@
> > +    }
> > +
> > +    bool growLength(uint32_t deltaLength)
> > +    {
> > +        MOZ_ASSERT(maxSize_.isSome() && deltaLength % wasm::PageSize == 0);
> 
> nit: we generally try to have 1 conjunct per assert, so I'd split this into
> two MOZ_ASSERT()s (since they're separate invariants) and put a \n after
> 

Fixed.

> @@ +505,5 @@
> > +        CheckedInt<uint32_t> curLength = actualByteLength();
> > +        CheckedInt<uint32_t> newLength = curLength + deltaLength;
> > +        MOZ_RELEASE_ASSERT(newLength.isValid());
> > +        // Guaranteed by Instance::growMemory
> > +        MOZ_ASSERT(newLength.value() <= maxSize_.value());
> 
> nit: There are two "Guaranteed by Instance::growMemory" comments; I'd have
> one, right before the first assert, with a \n before to separate from the
> CheckedInt<>s
> 

Fixed.

> @@ +536,5 @@
> > +        MOZ_RELEASE_ASSERT(deltaMaxSize % wasm::PageSize  == 0);
> > +
> > +        CheckedInt<uint32_t> curMax = maxSize_.value();
> > +        CheckedInt<uint32_t> newMax = curMax + deltaMaxSize;
> > +        MOZ_RELEASE_ASSERT(newMax.isValid());
> 
> Another nice release assert would be newMax.value() % wasm::PageSize == 0.
> 

Fixed.

> @@ +538,5 @@
> > +        CheckedInt<uint32_t> curMax = maxSize_.value();
> > +        CheckedInt<uint32_t> newMax = curMax + deltaMaxSize;
> > +        MOZ_RELEASE_ASSERT(newMax.isValid());
> > +
> > +        size_t legalNewMapped = wasm::LegalizeMapLength(newMax.value());
> 
> nit: there's not an un-legal newMapped, so I'd go with newMappedSize to make
> the destination clear.
> 

Fixed.

> @@ +565,5 @@
> >  {
> > +    size_t mappedSize = wasm::LegalizeMapLength(maxSize.valueOr(numBytes));
> > +
> > +    MOZ_RELEASE_ASSERT(mappedSize <= SIZE_MAX - gc::SystemPageSize());
> > +    MOZ_RELEASE_ASSERT(numBytes <= maxSize.valueOr(UINT32_MAX) && numBytes <= mappedSize);
> 
> I think this second assert is now fairly trivially implied by above.
> 

Fixed.

> @@ +570,2 @@
> >      MOZ_ASSERT(numBytes % wasm::PageSize == 0);
> > +    MOZ_ASSERT(mappedSize % wasm::PageSize == 0);
> 
> nit: I'd use gc::SystemPageSize() instead since that's the relevant property
> for mapping memory. Also technically I don't think legalize has to
> wasm::PageSize-align (e.g., if it uses a <64k guard page).
> 

Fixed.

> @@ +578,4 @@
> >      if (!data)
> >          return nullptr;
> >  
> > +    // Note we will waste a page on zero-sized memories here
> 
> nit: we're allocating a whole page, true, but that's just evident in the
> init of numBytesWithHeader, so I'd remove the comment (here and below).
> 

Fixed.

> @@ +603,4 @@
> >  #  endif
> >  
> > +    uint8_t* buffer = reinterpret_cast<uint8_t*>(data) + gc::SystemPageSize();
> > +    uint8_t* base = buffer - sizeof(WasmArrayRawBuffer);
> 
> nit: s/base/header/ then s/buffer/base/ to match field names in
> WasmArrayRawBuffer.
> 

Fixed.

> @@ +614,2 @@
> >  {
> > +    WasmArrayRawBuffer* rawBuf = (WasmArrayRawBuffer*)((uint8_t*)mem - sizeof(WasmArrayRawBuffer));
> 
> nit: s/rawBuf/header/
> 

Fixed.

> @@ +645,5 @@
> > +    // First try to map the maximum requested memory
> > +    void* data = WasmArrayRawBuffer::Allocate(numBytes, maxSize);
> > +    if (!data) {
> > +#ifdef  WASM_HUGE_MEMORY
> > +        return nullptr;
> 
> ReportOutOfMemory(cx)
> 

Fixed.

> @@ +659,5 @@
> > +
> > +        for (; cur > numBytes; cur = cur / 2) {
> > +            data = WasmArrayRawBuffer::Allocate(numBytes, Some(cur));
> > +
> > +            if (data)
> 
> nit: rm \n before test
> 

Fixed.

> @@ +668,5 @@
> >              ReportOutOfMemory(cx);
> >              return nullptr;
> >          }
> >  
> > +        WasmArrayRawBuffer *buf  = (WasmArrayRawBuffer*) data - sizeof(WasmArrayRawBuffer);
> 
> Can you have WasmArrayRawBufer::Allocate() return a WasmArrayRawBuffer*
> instead (and then the callers can call ->dataPointer() as needed
> 

Fixed.

> @@ +679,5 @@
> >  
> > +    BufferContents contents = BufferContents::create<WASM_MAPPED>(data);
> > +    ArrayBufferObject* buffer = ArrayBufferObject::create(cx, numBytes, contents);
> > +
> > +    if (!buffer) {
> 
> nit: rm \n before test
> 

Fixed.

> @@ +816,5 @@
> > +        return contents().wasmBuffer()->mappedSize();
> > +    else
> > +        // Can use byteLength() instead of actualByteLength since if !wasmMapped()
> > +        // then this is an asm.js buffer, and thus cannot grow.
> > +        return byteLength();
> 
> nit: { } around then and else branches b/c multiline
> 

Fixed.

> @@ +855,5 @@
> > +
> > +    // TODO: This is a hack just to get the SharedArrayBuffer tests passing for
> > +    // now. Should be fixed when we add proper support for sharing/growing
> > +    // memory with SAB
> > +    return wasmMappedSize();
> 
> nit: "hack" is a bit strong, how about: "TODO: when SharedArrayBuffer can be
> used from wasm, this should be replaced by
> SharedArrayBufferObject::wasmBoundsCheckLimit()"
> 

Fixed.

> ::: js/src/vm/ArrayBufferObject.h
> @@ +145,5 @@
> >      };
> >  
> >      enum BufferKind {
> >          PLAIN               = 0, // malloced or inline data
> > +        ASMJS_MALLOCED       = 1,
> 
> nit: indent of =
> 

Fixed.

> @@ +213,5 @@
> >          uint8_t* data() const { return data_; }
> >          BufferKind kind() const { return kind_; }
> >  
> >          explicit operator bool() const { return data_ != nullptr; }
> > +        WasmArrayRawBuffer *wasmBuffer() const;
> 
> nit: WasmArrayRawBuffer*

Fixed.
Attachment #8784621 - Attachment is obsolete: true
Attachment #8784621 - Flags: review?(sunfish)
Attachment #8784998 - Flags: review?(sunfish)
(Assignee)

Updated

2 years ago
Blocks: 1298202
Comment on attachment 8784998 [details] [diff] [review]
grow_mem_candidate_v40.diff

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

::: js/src/asmjs/WasmBinaryToText.cpp
@@ +412,5 @@
> +
> +    const char* opStr;
> +    switch (op.expr()) {
> +      case Expr::Nop:     opStr = "nop"; break;
> +      case Expr::CurrentMemory:     opStr = "current_memory"; break;

Uber-nit: Vertially-align these, as in PrintNullaryOperator above.

::: js/src/asmjs/WasmInstance.cpp
@@ +313,5 @@
> +uint32_t
> +Instance::currentMemory()
> +{
> +    MOZ_RELEASE_ASSERT(memory_);
> +    size_t curMemByteLen = memory_->buffer().wasmActualByteLength();

Nit: wasmActualByteLength returns a uint32_t. Promoting it to size_t here is safe, but makes it a little non-obvious that the subsequent conversion back to uint32_t is safe, so I'd recommend using uint32_t here.
Attachment #8784998 - Flags: review?(sunfish) → review+
(Assignee)

Comment 37

2 years ago
Created attachment 8785034 [details] [diff] [review]
grow_mem_candidate_v41.diff

Fixed final nits from Dan

(In reply to Dan Gohman [:sunfish] from comment #35)
> Comment on attachment 8784998 [details] [diff] [review]
> grow_mem_candidate_v40.diff
> 
> Review of attachment 8784998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/asmjs/WasmBinaryToText.cpp
> @@ +412,5 @@
> > +
> > +    const char* opStr;
> > +    switch (op.expr()) {
> > +      case Expr::Nop:     opStr = "nop"; break;
> > +      case Expr::CurrentMemory:     opStr = "current_memory"; break;
> 
> Uber-nit: Vertially-align these, as in PrintNullaryOperator above.
> 

Fixed.

> ::: js/src/asmjs/WasmInstance.cpp
> @@ +313,5 @@
> > +uint32_t
> > +Instance::currentMemory()
> > +{
> > +    MOZ_RELEASE_ASSERT(memory_);
> > +    size_t curMemByteLen = memory_->buffer().wasmActualByteLength();
> 
> Nit: wasmActualByteLength returns a uint32_t. Promoting it to size_t here is
> safe, but makes it a little non-obvious that the subsequent conversion back
> to uint32_t is safe, so I'd recommend using uint32_t here.

Fixed.
Attachment #8784998 - Attachment is obsolete: true
(Assignee)

Comment 38

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1cf1e462ff9567386a33d7288bfc6c6a473f159
Bug 1287967 - Baldr: Add current_memory and grow_memory (r=luke, sunfish)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e89f8bd9bc - apparently there's something else you have to do to avoid breaking shell builds like https://treeherder.mozilla.org/logviewer.html#?job_id=34719742&repo=mozilla-inbound
Also, a rooting hazard: https://treeherder.mozilla.org/logviewer.html#?job_id=34719761&repo=mozilla-inbound

Also, still busted on the backout, which would ordinarily make me say it needs a clobber, both ways, but since the clobberer doesn't actually work for taskcluster builds, we're just sort of stuck busted.
Also https://treeherder.mozilla.org/logviewer.html#?job_id=34722269&repo=mozilla-inbound - you apparently either shut off wasm on Android, or made that test for it think that you did.
(Assignee)

Comment 42

2 years ago
(In reply to Phil Ringnalda (:philor) from comment #41)
> Also
> https://treeherder.mozilla.org/logviewer.html#?job_id=34722269&repo=mozilla-
> inbound - you apparently either shut off wasm on Android, or made that test
> for it think that you did.

Thank you! Recording rootcause:

With this patch we require signal handling for WASM to work. Due to a known bug ( https://android-review.googlesource.com/#/c/52333) we disable signal handling on Android prior to 4.4. We probably need to disable wasm tests on android 4.3.

Comment 43

2 years ago
Ah, and jit-tests don't fail because we query wasmIsSupported() and quit() if not.  Probably instead we should just have the WebAssembly object just not be exposed and then short-circuit the test if it's not.  I can write a patch to do this.
(Assignee)

Comment 44

2 years ago
Created attachment 8785515 [details] [diff] [review]
grow_mem_candidate_v42.diff

There were several issues causing the backout of the previous patch. This patch fixes those as listed below:

1) Test failure on Android 4.3 API15+ debug for test_webassembly_compile.html
Cause: With this patch platforms not supporting signals (i.e. Android before 4.4) don't get wasm.

Solution: Check wasmIsSupported in this test

2) Assertion failures running wasm tests in Arm simulator:
Assertion failure: length % PageSize == 0, at /builds/slave/m-in_lx-d_sm-arm-sim-000000000/src/js/src/asmjs/WasmTypes.cpp:632 

This was happening when calling RoundUpToNextValidARMLengthImmediate(0). We would call mozilla::RoundUpPow2(0); which seems to return 1 instead, which is not a multiple of PageSize.

Solution: return length when length == 0 in RoundUpToNextValidARMLengthImmediate

3) Build failures for test SM-tc(..nu..): error: 'HaveSignalHandlers' is not a member of 'js::wasm'.
Cause: WasmJS.cpp doesn't explicitly include WasmSignalHandlers.h. I am guessing on other build configs it was being included for other reasons?
Solution: Include WasmSignalHandlers.h in WasmJS.cpp

4) Rooting hazards in WasmJS.cpp
Cause: maximumAtom was not rooted across a GetProperty call.
Solution: root it :)

Remaining unresloved: Build failure for SM-tc(cgc) due to old-configure.in changes. 

P.S. Note that this is diff No. 42. The answer to life :)
Attachment #8785034 - Attachment is obsolete: true
Attachment #8785515 - Flags: review?(sunfish)
Attachment #8785515 - Flags: review?(luke)
Comment on attachment 8785515 [details] [diff] [review]
grow_mem_candidate_v42.diff

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

Great work on all of this! When this is good to go, can you please also remove the TODOs and the calls to quit() in jit-tests/wasm/spec/{resizing,memory_trap}.wast.js? (and check they pass with your patch)

Comment 46

2 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #45)
> can you please also remove the TODOs and the calls to quit() in
> jit-tests/wasm/spec/{resizing,memory_trap}.wast.js? (and check they pass
> with your patch)

This patch takes the first step by allowing resizing when max is declared (so that resize = mprotect); another patch is necessary to support resizing when max is not (where resize = realloc).

Comment 47

2 years ago
Created attachment 8786139 [details] [diff] [review]
fix-simulator

With the wasm::PageSize assert fixed, we need to teach the ARM simulator about faults.  We can't quite pull a trick like we do for the interrupt fault since we're actually crashing in C++.  Instead, this patch catches the fault ahead of time in the simulator and preemptively redirects control flow to the outOfBounds handler.
Attachment #8786139 - Flags: review?(sunfish)

Comment 48

2 years ago
Comment on attachment 8785515 [details] [diff] [review]
grow_mem_candidate_v42.diff

The changes look good, but I'll need to fold in the simulator patch to land this.
Attachment #8785515 - Flags: review?(luke) → review+
Comment on attachment 8786139 [details] [diff] [review]
fix-simulator

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

Looks good.

::: js/src/jit/arm/Simulator-arm.cpp
@@ +1498,5 @@
>      exclusiveMonitorHeld_ = false;
>  }
>  
> +bool
> +Simulator::handleWasmFault(int32_t addr, unsigned numBytes)

It'd be good to have a brief comment hereabouts, for the benefit of someone reading the simulator code who might not know what wasm is up to here.

::: js/src/jit/arm/Simulator-arm.h
@@ +104,5 @@
>  
>      static void Destroy(Simulator* simulator);
>  
>      // Constructor/destructor are for internal use only; use the static methods above.
> +    Simulator(JSContext* cx);

Add an `explicit` keyword.
Attachment #8786139 - Flags: review?(sunfish) → review+

Comment 50

2 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6fddb22a8b5
Baldr: Add current_memory and grow_memory (r=luke,sunfish)

Comment 51

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f6fddb22a8b5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

2 years ago
Depends on: 1299330
Depends on: 1300202
Depends on: 1300898
Attachment #8785515 - Flags: review?(sunfish)
You need to log in before you can comment on or make changes to this bug.