Closed Bug 1239177 Opened 4 years ago Closed 4 years ago

Odin: make calls more like wasm

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(2 files, 3 obsolete files)

In particular, this bug is to avoid storing the Sig* in the bytecode for calls and instead just stores an index (into the table of functions, imports or signatures).
The challenge of course is that the indices in the wasm bytecode need to index arrays that are being concurrently grown by the thread generating the wasm.  The elements are always written before accessed, but the memory holding the elements itself can move when the Vector resizes.  The first approach I thought of to make a little SharedField<T> wrapper that wrapped and locked each Vector so that the resizing was atomic.  I decided against this in lieu of the next approach.
Attached patch approach 2 (obsolete) — Splinter Review
Since the only problem is the resizing of the arrays and since one can come up with reasonable upper-bounds on the number of functions, imports, signatures and function-pointer tables, a simpler approach was to have fixed-size arrays and fail if the size is exceeded.  The arrays only live as long as compilation and, currently consume 4.8mb on 32-bit, 9.2mb on 64-bit which is far less than the memory spent on MIR/LIR.
Attachment #8707280 - Flags: review?(bbouvier)
Attached patch approach 2 (obsolete) — Splinter Review
Attachment #8707280 - Attachment is obsolete: true
Attachment #8707280 - Flags: review?(bbouvier)
Attachment #8707323 - Flags: review?(bbouvier)
Comment on attachment 8707323 [details] [diff] [review]
approach 2

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

Thank you for doing this.

::: js/src/asmjs/WasmGenerator.cpp
@@ +105,5 @@
>      return !cx->isJSContext() || cx->asJSContext()->runtime()->canUseOffthreadIonCompilation();
>  }
>  
>  bool
> +ModuleGenerator::init(AsmJSBool isAsmJS)

nit: isAsmJS is unused

@@ +189,5 @@
> +        MOZ_ASSERT(*shared_->sigs[*index] == sig);
> +        return true;
> +    }
> +
> +    *index = sigToIndex_.count();

Maybe MOZ_ASSERT(*index == declaredSigs_.length()), for sanity?

@@ +198,5 @@
> +
> +    if (!sigToIndex_.add(p, declaredSig.get(), *index))
> +        return false;
> +
> +    if (*index > MaxSigs)

This check could be made above, just after *index is set?

::: js/src/asmjs/WasmGenerator.h
@@ +61,5 @@
> +
> +struct SharedGeneratorState
> +{
> +    // Initialized zero via pod_calloc:
> +    const DeclaredSig* sigs[MaxSigs];

All these arrays could be mozilla::Array, for more free checks (operator[]) and ease of use (for-of loops ftw). but this is your call.
In this case, you could then be able to use ArrayLength and other helpers in the code manipulating lengths, instead of using the raw MaxSigs et al. constants.

@@ +67,5 @@
> +    const DeclaredSig* importSigs[MaxImports];
> +    uint32_t           importGlobalDataOffsets[MaxImports];
> +};
> +
> +// The ModuleConsumer presents a restricted, read-only view on the shared

Consumer feels weird to me, because consuming is taking something that won't be there anymore once it's been taken. Although in this case, signatures are not Move'd once they're observed. Really, this is just observing the signatures. How about ModuleObserver? SharedGeneratorStateObserver? SharedStateObserver? SignaturesObserver? (global data offsets being here considered a detail; I'd guess in this case we can also call the SharedGeneratorState something else like SharedSignatures)

@@ +79,5 @@
> +  public:
> +    explicit ModuleConsumer(const SharedGeneratorState& shared)
> +      : shared_(shared)
> +    {}
> +    const DeclaredSig& sig(uint32_t sigIndex) {

Can all methods be const in this object?

::: js/src/asmjs/WasmModule.cpp
@@ +855,5 @@
>  
>  
>  Module::Module(UniqueModuleData module, AsmJSBool isAsmJS)
>    : module_(Move(module)),
> +    isAsmJS_(IsAsmJS(isAsmJS)),

nice

::: js/src/asmjs/WasmTypes.h
@@ +208,2 @@
>  
> +    bool init(const Sig& rhs) {

Naming is weird: from an outsider's perspective, it looks like rhs will be used internally as a const&, although it's copied. How about simply naming this "copy"? Or even better, "clone"?

@@ +211,4 @@
>          MOZ_ASSERT(args_.empty());
> +        return args_.appendAll(rhs.args_);
> +    }
> +    Sig& operator=(Sig&& rhs) {

How about deleting the copy ctor and copy assignment operators? I guess trying to use them will trigger an error because ValVector can't be copied, but that might be better to make this very explicit from the start.
Attachment #8707323 - Flags: review?(bbouvier) → review+
Attached patch approach 2 (take 2) (obsolete) — Splinter Review
Thanks!  This version addresses all feedback and changes the shared struct to instead store Vectors that are initialized by the client of ModuleGenerator so that AsmJS.cpp can inflate them to the max size whereas wasm can fit them to the exact declared size.
Attachment #8707323 - Attachment is obsolete: true
Attachment #8708148 - Flags: review?(bbouvier)
This version moves the unification of signatures to MV from MG b/c wasm doesn't want that: it just hands in the vector of signatures.
Attachment #8708148 - Attachment is obsolete: true
Attachment #8708148 - Flags: review?(bbouvier)
Attachment #8708168 - Flags: review?(bbouvier)
Comment on attachment 8708168 [details] [diff] [review]
approach 2 (take 3)

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

Looks good! Two remarks about TSAN testing, and having a unique vector containing all import's information, but this shouldn't block landing. Thank you for the patch.

::: js/src/asmjs/AsmJS.cpp
@@ +1559,5 @@
> +    struct SigHashPolicy
> +    {
> +        typedef const Sig& Lookup;
> +        static HashNumber hash(Lookup sig) { return sig.hash(); }
> +        static bool match(const DeclaredSig* lhs, Lookup rhs) { return *lhs == rhs; }

(pre-existing, but it's very neat that DeclaredSig is a pointer and Lookup isn't, so that we don't risk to compare pointers by mistake, as previously done :-))

@@ +1591,3 @@
>          }
> +        static bool match(NamedSig lhs, Lookup rhs) {
> +            return lhs.name_ == rhs.name && *lhs.sig_ == rhs.sig;

pre-existing and not mandatory at all, but this could use SighHashPolicy::match for comparing the signatures

@@ +2077,5 @@
>          }
> +        *importIndex = module_->imports.length();
> +        if (!module_->imports.emplaceBack(ffiIndex))
> +            return false;
> +        if (*importIndex >= MaxImports)

This check could be made right after *importIndex is set.

@@ +4572,5 @@
>        case ExprType::F32x4:  if (!f.writeOp(Expr::F32X4CallImport)) return false; break;
>        case ExprType::B32x4:  if (!f.writeOp(Expr::B32X4CallImport)) return false; break;
>      }
>  
>      // Global data offset

This comment needs an update

::: js/src/asmjs/WasmGenerator.cpp
@@ +115,4 @@
>      if (!link_)
>          return false;
>  
> +    MOZ_ASSERT(shared->importSigs.length() == shared->importGlobalDataOffsets.length());

Why not having a single Vector of struct {Signature, globalDataOffset} instead? Fewer vectors ought to mean less memory, and it's nice to have in case we'd want to add data to imports in the future.

@@ +278,5 @@
> +}
> +
> +const DeclaredSig&
> +ModuleGenerator::sig(uint32_t index) const
> +{

Maybe MOZ_ASSERT(shared_->sigs[sigIndex] != Sig()); ?

@@ +419,3 @@
>  {
>      MOZ_ASSERT(activeFunc_ == fg);
> +    MOZ_ASSERT(shared_->funcSigs[funcIndex]);

The call to funcSig() below will assert this already, so this one can be removed.

@@ +422,3 @@
>  
> +    UniqueFuncBytecode func =
> +        cx_->make_unique<FuncBytecode>(fg->name_,

Can you use MakeUnique here too?

::: js/src/asmjs/WasmGenerator.h
@@ +50,5 @@
>  
> +// The ModuleGeneratorData holds all the state shared between the
> +// ModuleGenerator and ModuleGeneratorThreadView. The ModuleGeneratorData is
> +// encapsulated by ModuleGenerator/ModuleGeneratorThreadView classes which
> +// present a race-free interface to the code in each thread. In particular,

Is TSAN likely to complain about these races? Any way to ensure it doesn't? (it sounds a bit sad to have to use Atomic for a vector though...)

@@ +52,5 @@
> +// ModuleGenerator and ModuleGeneratorThreadView. The ModuleGeneratorData is
> +// encapsulated by ModuleGenerator/ModuleGeneratorThreadView classes which
> +// present a race-free interface to the code in each thread. In particular,
> +// while the whole arrays are concurrently read/written, any given element is
> +// initialized by the ModuleGeneratorThreadView thread before an index to that

nit: ModuleGenerator
Attachment #8708168 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> > +    MOZ_ASSERT(shared->importSigs.length() == shared->importGlobalDataOffsets.length());
> 
> Why not having a single Vector of struct {Signature, globalDataOffset}
> instead? Fewer vectors ought to mean less memory, and it's nice to have in
> case we'd want to add data to imports in the future.

> Maybe MOZ_ASSERT(shared_->sigs[sigIndex] != Sig()); ?

Sig() is a valid (void()) signature.

> Why not having a single Vector of struct {Signature, globalDataOffset} instead? Fewer vectors
> ought to mean less > memory, and it's nice to have in case we'd want to add data to imports in 
> the future.

Although the actual Vector itself takes up a few bytes, the actual arrays will be smaller with the two homogeneous arrays on 64-bit (b/c one field is a uint32 and the other is a pointer).  However, the difference should be small for imports and it is cleaner to have a single 'imports' Vector so I'll do that for now.

> Is TSAN likely to complain about these races? Any way to ensure it doesn't?
> (it sounds a bit sad to have to use Atomic for a vector though...)

There won't be a race on any individual word so TSAN shouldn't have anything to report.  I'll update the comment to be a bit more clear though.
https://hg.mozilla.org/mozilla-central/rev/20619c132abb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.