1412238 - Implement WebAssembly import/export of mutable globals

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[Lars T Hansen [:lth]]

The mutable globals import/export proposal is not yet quite a proposal: https://github.com/WebAssembly/threads/blob/master/proposals/threads/Globals.md.  

There's some noise that threads won't be useful until this lands but I doubt that's true; threads+shared libraries might be more of an issue though.

Comment 1

[Lars T Hansen [:lth]]

According to Luke the CG decided we need this for the threading MVP; go forth and code, I guess.

Comment 2

[Lars T Hansen [:lth]]

Before going much further here we need to nail down the spec, which still feels pretty flimsy, there are several open issues, and even the name 'Global' is disputed.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1412238-wasm-global-sketch.patch

Sketch implementation of WebAssembly.Global, does not handle mutable globals yet.  Intended for experimenting with webcompat fallout from exporting this type.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1412238-wasm-global-hacks.patch

Code that wraps a value in a WebAssembly.Global on export, and unwraps a value from a WebAssembly.Global on input.  Likely hacky and wrong in subtle ways.

Also, test case adjustments since assertEq and other predicates look carefully at types and can distinguish a WebAssembly.Global from its value, even though casual observers won't see the difference.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1412238-wasm-global-hacks.patch

Updated with slightly better test cases and ifdeffery.

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8941454 [details] [diff] [review]
bug1412238-wasm-global-sketch.patch

Asking for review for the purposes of running the experiment promised here: https://github.com/WebAssembly/threads/issues/73#issuecomment-347613146.  WebAssembly.Global would be enabled on Nightly only.  The present patch only introduces the new type with the required functionality, but does not make use of it; see next patch for that.

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 8941776 [details] [diff] [review]
bug1412238-wasm-global-hacks.patch

Again, r? for the purposes of running the experiment promised in
https://github.com/WebAssembly/threads/issues/73#issuecomment-347613146.  This gives the new type teeth by accepting it in an import descriptor and producing it on the export object.  The production on the export object can be controlled independently with a separate ifdef, which allows us to mitigate problems very quickly at low risk.

(It's still open how we would assess the experiment, short of looking for bug reports from people running into problems.  Some kind of PR may be warranted.)

Comment 8

[Lars T Hansen [:lth]]

Looks like I also have to fix a few web-platform tests to be more lenient about equality.

Comment 9

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55527467dcbbe5708825d46fa1d1e428f256c86f

Comment 10

[Lars T Hansen [:lth]]

Attachment: bug1412238-wasm-global-wpt-adjust.patch

Adjust web-platform-tests too.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8941454 [details] [diff] [review]
bug1412238-wasm-global-sketch.patch

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

Patch looks nice and clean, but for it to not break web code, don't we need the @@toPrimitive hook that lets a Global object be used in numeric contexts?  It'd be nice to have few tests for this (both Global and the @@toPrimitive hook).

::: js/src/wasm/WasmJS.cpp
@@ +1902,5 @@
> +{
> +    "WebAssembly.Global",
> +    JSCLASS_DELAY_METADATA_BUILDER |
> +    JSCLASS_HAS_RESERVED_SLOTS(WasmGlobalObject::RESERVED_SLOTS) |
> +    JSCLASS_FOREGROUND_FINALIZE,

Depending on how the mutable global cells are implemented, this might be background finalizable.

@@ +1954,5 @@
> +    RootedId typeId(cx, AtomToId(typeAtom));
> +
> +    RootedValue typeVal(cx);
> +    if (!GetProperty(cx, obj, obj, typeId, &typeVal))
> +        return false;

For here and "mutable" and "value", could you use the static GetProperty() overload above that takes a const char* and returns the gotten value?  (I see a few other places have neglected to use which could be converted to use it.)

@@ +1965,5 @@
> +    if (!typeStr)
> +        return false;
> +
> +    ValType globalType;
> +    if (JS_FlatStringEqualsAscii(typeStr, "i32")) {

For checking string equality, I'd copy what WasmTableObject::construct() does for "anyfunc":
 1. require the type to be a string instead of doing the ToString() coercion
 2. do typeVal.toString()->ensureLinear(); "linear" is a weaker requirement than "flat" (linear = not rope, has array of chars; flat = null terminated)
 3. use StringEqualsAscii()

@@ +2007,5 @@
> +        break;
> +      }
> +      case ValType::F32:
> +      case ValType::F64: {
> +        // TODO: Possible we want to do an additional rounding step for f32?

I'm pretty sure this will be speced as ToWebAssemblyValue().  ToWebAssemblyValue() is currently used in the JS spec (https://webassembly.github.io/spec/js-api/index.html) twice: JS args passed to exported wasm function, and JS values passed to global import.  Embarrassingly, we seem to have open-coded both.  Could you factor out a ToWebAssemblyValue(JSContext*, HandleValue, ValType, Val* out) and use it in all (now 3) places?

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8941776 [details] [diff] [review]
bug1412238-wasm-global-hacks.patch

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

::: js/src/jit-test/tests/wasm/globals.js
@@ +119,4 @@
>  )`, { globals: {x: 42} }).exports;
>  
>  assertEq(module.getter(), 42);
> +assertEq(Number(module.value), 42);

IIUC, with the @@toPrimitive change suggested in the other patch, these explicit Number() coercions wouldn't be necessary, right?  (Actually, how are they working now?)

::: js/src/moz.build
@@ +646,5 @@
> +    # An experiment we want to run on Nightly: Can we change the JS
> +    # representation of an exported global from the global's value
> +    # to an instance of WebAssembly.Global without breaking existing
> +    # wasm content?
> +    DEFINES['ENABLE_WASM_GLOBAL_EXPORT_EXPERIMENT'] = True

This is nice and explicit, but what if we just made ENABLE_WASM_GLOBAL *be* the experiment, and gave it exactly the proposed semantics we're experimenting with?  That way we're not having to think about what it means with the two flags in all the possible states.

Comment 13

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #11)
> Comment on attachment 8941454 [details] [diff] [review]
> bug1412238-wasm-global-sketch.patch
> 
> Review of attachment 8941454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks nice and clean, but for it to not break web code, don't we need
> the @@toPrimitive hook that lets a Global object be used in numeric
> contexts? 

There is such a hook.  I have tested locally, but you're right I should add some test cases.

> 
> @@ +2007,5 @@
> > +        break;
> > +      }
> > +      case ValType::F32:
> > +      case ValType::F64: {
> > +        // TODO: Possible we want to do an additional rounding step for f32?
> 
> I'm pretty sure this will be speced as ToWebAssemblyValue(). 
> ToWebAssemblyValue() is currently used in the JS spec
> (https://webassembly.github.io/spec/js-api/index.html) twice: JS args passed
> to exported wasm function, and JS values passed to global import. 
> Embarrassingly, we seem to have open-coded both.  Could you factor out a
> ToWebAssemblyValue(JSContext*, HandleValue, ValType, Val* out) and use it in
> all (now 3) places?

Sure thing.

Comment 14

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #12)
> Comment on attachment 8941776 [details] [diff] [review]
> bug1412238-wasm-global-hacks.patch
> 
> Review of attachment 8941776 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/wasm/globals.js
> @@ +119,4 @@
> >  )`, { globals: {x: 42} }).exports;
> >  
> >  assertEq(module.getter(), 42);
> > +assertEq(Number(module.value), 42);
> 
> IIUC, with the @@toPrimitive change suggested in the other patch, these
> explicit Number() coercions wouldn't be necessary, right?  (Actually, how
> are they working now?)

No, that's wrong.  There is such a hook and it is working, but assertEq is careful not to invoke it - it looks at types first, then at values.

> ::: js/src/moz.build
> @@ +646,5 @@
> > +    # An experiment we want to run on Nightly: Can we change the JS
> > +    # representation of an exported global from the global's value
> > +    # to an instance of WebAssembly.Global without breaking existing
> > +    # wasm content?
> > +    DEFINES['ENABLE_WASM_GLOBAL_EXPORT_EXPERIMENT'] = True
> 
> This is nice and explicit, but what if we just made ENABLE_WASM_GLOBAL *be*
> the experiment, and gave it exactly the proposed semantics we're
> experimenting with?  That way we're not having to think about what it means
> with the two flags in all the possible states.

Sure, we can do that.  (Though in the limit, we'll want to land wasm globals but not this particular experiment, which is why there are two defines now.)

Comment 15

[Luke Wagner [:luke]]

(In reply to Lars T Hansen [:lth] from comment #13)
> > Patch looks nice and clean, but for it to not break web code, don't we need
> > the @@toPrimitive hook that lets a Global object be used in numeric
> > contexts? 
> 
> There is such a hook.

Ah hah, I see it now in methods[]; I had been looking for a toPrimitive method on the class but I see you're able to simply reuse the value getter.

(In reply to Lars T Hansen [:lth] from comment #14)
> No, that's wrong.  There is such a hook and it is working, but assertEq is
> careful not to invoke it - it looks at types first, then at values.

Makes sense now, thanks!

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8941454 [details] [diff] [review]
bug1412238-wasm-global-sketch.patch

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

Right, so r+ with comments addressed and @@toPrimitive tests added.

Comment 17

[Lars T Hansen [:lth]]

Attachment: bug1412238-ToWebAssemblyValue.patch

Is this roughly what you had in mind for ToWebAssemblyValue?  Note,

(1) it is also useful to have ToJSValue, so it is here

(2) callExport does not translate all that easily to ToWebAssemblyValue because it also handles all the asm.js simd types, but it can be done

(3) the Value -> Val -> Value roundtrip in the WebAssembly.Global constructor looks weird but both converts and canonicalizes

(4) ToWebAssemblyValue should not check for i64 or Number, the caller must do that per spec, and ToWebAssemblyValue may do ToNumber and similar

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8944394 [details] [diff] [review]
bug1412238-ToWebAssemblyValue.patch

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

(In reply to Lars T Hansen [:lth] from comment #17)
> (3) the Value -> Val -> Value roundtrip in the WebAssembly.Global
> constructor looks weird but both converts and canonicalizes

Yes, I think this is exactly right since we're basically precomputing and caching the JS value returned by the getter and the getter would do a ToJSValue() from a wasm value, by spec.

::: js/src/wasm/WasmJS.cpp
@@ +2015,5 @@
>      if (!JS_GetProperty(cx, obj, "value", &valueVal))
>          return false;
>  
> +    Val globalValue_;
> +    if (!ToWebAssemblyValue(cx, globalType, valueVal, &globalValue_))

nit: stylistically, I like trailing _ to exclusively indicate "member variable"; could you perhaps find another way to distinguish the two values?  (e.g., wasmGlobalValue/jsGlobalValue)

::: js/src/wasm/WasmJS.h
@@ +43,5 @@
>  bool
>  HasSupport(JSContext* cx);
>  
> +bool
> +ToWebAssemblyValue(JSContext* cx, ValType targetType, HandleValue v, Val* val);

nit: could you put a comment before this (with \n before and after comment) saying something short to the effect of "Spec-defined conversion functions"?

Comment 19

[Lars T Hansen [:lth]]

Attachment: bug1412238-wasm-global-rollup.patch

Rollup of patches, carrying Luke's r+.

Comment 20

[Lars T Hansen [:lth]]

Attachment: bug1412238-wasm-global-wpt-adjust-v2.patch

We're landing a Nightly-only experiment that changes an API in a way we believe to be benign in practice (see other patch).  This requires a WPT adjustment: to pass testing with the new API; also to not affect beta and later as the test case remains enabled on those branches while the API change is disabled.

Comment 21

[James Graham [:jgraham]]

Comment on attachment 8944466 [details] [diff] [review]
bug1412238-wasm-global-wpt-adjust-v2.patch

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

This looks fine, but I can't in all honesty claim to be an appropriate reviewer for the test change.

Comment 22

[Lars T Hansen [:lth]]

Thanks, James, that's fine - I have an r+ on the corresponding jit-test case, so I'll just assume this is going to be OK.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dee3b871181
Implement WebAssembly.Global for immutables. r=luke

Comment 24

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/5dee3b871181

Comment 25

[Lars T Hansen [:lth]]

Attachment: bug1412238-wasm-global-wip.patch

Wip for remaining work items.  Done: spec conformance, object identity of WebAssembly.Global, cell storage.  Missing: actually indirecting to the cell for mutable globals that are observable.

Comment 26

[Lars T Hansen [:lth]]

Attachment: bug1412238-wasm-global-adjust-semantics.patch

Minor adjustments to what we have already landed, see commit msg.

Comment 27

[Lars T Hansen [:lth]]

Attachment: bug1412238-wasm-global-mutable-with-object-identity.patch

Mutable importable/exportable globals.  See commit msg for technical details.

One seemingly unmotivated change here is in Registers.h where a field of RegisterOrSP has become non-const.  RegisterOrSP was never meant to hinder assignability of Address, but with the const field it does, and for the Ion changes that was bad.  If desired I can try to hide the field better.

Comment 28

[Lars T Hansen [:lth]]

Attachment: bug1412238-wasm-global-mutable-with-object-identity-v2.patch

As discussed, this uses a GCVector rather than a GCHashMap, and only retains those vector elements that represent indirect globals.  It also compresses that vector.

One thing that grates on me here is that I can't size that GCVector when I allocate it, to do so I'd have to loop over the globals (I think) to find the highest numbered global that needs representing, or size it to contain all globals.  Neither is particularly elegant.  Instead, it is resized on demand.

Comment 29

[Luke Wagner [:luke]]

Comment on attachment 8960939 [details] [diff] [review]
bug1412238-wasm-global-mutable-with-object-identity-v2.patch

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

Yay, great work!  r+ assuming these issues are fixed as suggested.

::: js/src/jit/CodeGenerator.cpp
@@ +7154,5 @@
>      Address addr(tls, offsetof(wasm::TlsData, globalArea) + mir->globalDataOffset());
> +    if (mir->isIndirect()) {
> +        Register tmp = ToRegister(ins->addrTemp());
> +        masm.loadPtr(addr, tmp);
> +        addr = Address(tmp, 0);

Instead of doing the indirection as part of the MWasm(Load|Store)GlobalVar op, which necessarily does two loads on every global read/write, could you use MWasmLoadTls to load the address of the global since MWasmLoadTls is super-GVNable.  I think this should be pretty helpful for common cases like: "STACKTOP += 4; i32.load (STACKTOP)".

Specifically, it looks like MWasm(Load|Store)GlobalVar would take either a (tls, globalDataOffset) or (address-of-global).  Changing globalDataOffset to be relative to tls, instead of (tls+offsetof(TlsData, globalArea)), would allow both cases to be collapsed into one (ptr, offset).

::: js/src/wasm/WasmInstance.cpp
@@ +456,5 @@
>          switch (global.kind()) {
>            case GlobalKind::Import: {
> +            if (global.isIndirect()) {
> +                const WasmGlobalObject* globalObj = globalObjs[global.importIndex()];
> +                const void* address = globalObj->cell();

nit: since globalAddr is necessarily word-aligned, how about a one-liner (here and below):
  *(void**)globalAddr = globalObjs[global.importIndex()]->cell();

@@ +470,5 @@
>                case InitExpr::Kind::Constant: {
> +                if (global.isIndirect()) {
> +                    const WasmGlobalObject* globalObj = globalObjs[i];
> +                    const void* address = globalObj->cell();
> +                    memcpy(globalAddr, &address, sizeof(address));

Shouldn't this be accompanied by a 'init.val().writePayload(address)'?  Could you add a test for this case?

@@ +479,5 @@
>                }
>                case InitExpr::Kind::GetGlobal: {
>                  const GlobalDesc& imported = metadata().globals[init.globalIndex()];
> +                if (imported.isIndirect()) {
> +                    MOZ_ASSERT(global.isIndirect());

IIUC, this case corresponds to this example:
 (module
  (global $g1 (import "foo" "bar") (mut i32))
  (global $g2 i32 (get_global $g1)))
I don't see why $g2 is necessarily indirect: it isn't imported or exported.  It seems like you need to handle both cases here.  Could you add a test case?

@@ +484,5 @@
> +                    const WasmGlobalObject* importedObj = globalObjs[imported.importIndex()];
> +                    const void* address = importedObj->cell();
> +                    memcpy(globalAddr, &address, sizeof(address));
> +                }
> +                else if (global.isIndirect()) {

nit: } else if (...) {

::: js/src/wasm/WasmJS.cpp
@@ +194,1 @@
>             ValVector* globalImports)

nit: Could you rename globalImports to globalImportValues to match WasmJS.h?

@@ +1007,3 @@
>                             SharedTableVector&& tables,
>                             Handle<FunctionVector> funcImports,
>                             const ValVector& globalImports,

nit: globalImportValues

@@ +2045,2 @@
>  /* static */ WasmGlobalObject*
> +WasmGlobalObject::create(JSContext* cx, const wasm::Val& val, bool isMutable)

nit: is 'wasm::' necessary?

@@ +2127,4 @@
>      if (!ToWebAssemblyValue(cx, globalType, valueVal, &globalVal))
>          return false;
>  
> +    Rooted<WasmGlobalObject*> global(cx, WasmGlobalObject::create(cx, globalVal, isMutable));

nit: I think you can just call 'create()' directly?

@@ +2175,5 @@
>      }
>  
> +    // i64 globals cannot be exported to JS and cannot be created from JS, and
> +    // so cannot appear here as the receiver of a new value.
> +    MOZ_ASSERT(global->type() != ValType::I64);

I think we should move the I64 failure here (and valueGetterImpl) from GetGlobalExport().  This is actually a big benefit of WebAssembly.Global: it allows a JS module loader to unconditionally connect the exports of one wasm module to the imports of another since, in that case, no setter/getter is getting called.  I think this also matches what the spec is saying.  Could you also add tests for this cases?

@@ +2189,5 @@
> +      case ValType::F64: cell->f64 = val.f64(); break;
> +      default:           MOZ_CRASH();
> +    }
> +
> +    return true;

nit: could you args.rval().setUndefined() here?

::: js/src/wasm/WasmModule.cpp
@@ +1017,5 @@
>  }
>  
> +static void
> +ExtractGlobalValue(const ValVector& globalImports, uint32_t globalIndex, const GlobalDesc& global,
> +                   Val* val)

nit: I think it would tidy up both callsites if this function returned Val by value.

@@ +1047,5 @@
> +}
> +
> +static bool
> +GetGlobalExport(JSContext* cx,
> +                HandleWasmInstanceObject instanceObj,

nit: 'instanceObj' param seems dead

@@ +1158,3 @@
>                      HandleWasmTableObject tableImport,
>                      HandleWasmMemoryObject memoryImport,
>                      const ValVector& globalImports,

nit: globalImportValues

@@ +1244,4 @@
>      if (!debug)
>          return false;
>  
> +#if defined(ENABLE_WASM_GLOBAL) && defined(EARLY_BETA_OR_EARLIER)

nit: could you factor this whole hunk of code into a new instantiateGlobals() method and put the call to it right after the other three instantiateX() methods above?

@@ +1318,5 @@
> +                globalObjs[packed] = globalObjs[next];
> +            packed++;
> +        }
> +
> +        if (!globalObjs.resize(packed))

I think this resize() call won't ultimately release the underlying allocation of the GCVector; you'd need podResizeToFit().  Since there's probably a separate small (or empty) allocation and copy (under the hood) anyway, what if instead we:
 - pass around a WasmGlobalObjectVector& everywhere (instead of the MutableHandle<UniquePtr<WithFries<WasmGlobalObjectVector>>>)
 - WasmInstanceObject::create() creates its own local copy of only the indirect globals (renaming WasmInstanceObject::globals() to indirectGlobals() to avoid confusion)

As a nice bonus (and my primary motivation), this avoids the implicit ordering dependency we have here where packing has to happen after the last time globalObjs is indexed with a globalIndex.  This is particularly trixie because WasmInstanceObject::globals() would return a vector that has very different meanings at different program points.

::: js/src/wasm/WasmTypes.h
@@ +70,5 @@
>  typedef Handle<WasmTableObject*> HandleWasmTableObject;
>  typedef MutableHandle<WasmTableObject*> MutableHandleWasmTableObject;
>  
> +class WasmGlobalObject;
> +typedef GCVector<HeapPtr<WasmGlobalObject*>, 8, SystemAllocPolicy> WasmGlobalObjectVector;

nit: could you add a RootedWasmGlobalObject typedef and use it in the patch?

::: js/src/wasm/WasmValidate.cpp
@@ +1262,4 @@
>              return false;
>          if (!GlobalIsJSCompatible(d, type, isMutable))
>              return false;
> +        if (!env->globals.append(GlobalDesc(ModuleKind::Wasm, type, isMutable,

nit: as a general style in other functions, I like to have ModuleKind::Wasm be the default argument so that only the AsmJS case is verbose

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5502d567e3f5
WebAssembly.Global semantics adjustments. r=luke

Comment 31

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #29)
> Comment on attachment 8960939 [details] [diff] [review]
> bug1412238-wasm-global-mutable-with-object-identity-v2.patch
> 
> Review of attachment 8960939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +470,5 @@
> >                case InitExpr::Kind::Constant: {
> > +                if (global.isIndirect()) {
> > +                    const WasmGlobalObject* globalObj = globalObjs[i];
> > +                    const void* address = globalObj->cell();
> > +                    memcpy(globalAddr, &address, sizeof(address));
> 
> Shouldn't this be accompanied by a 'init.val().writePayload(address)'? 
> Could you add a test for this case?

I do have tests and they pass :) The trick is that when we instantiate the globals we create the cell and write the value into the cell at that point, so we don't have to do so here.

> @@ +479,5 @@
> >                }
> >                case InitExpr::Kind::GetGlobal: {
> >                  const GlobalDesc& imported = metadata().globals[init.globalIndex()];
> > +                if (imported.isIndirect()) {
> > +                    MOZ_ASSERT(global.isIndirect());
> 
> IIUC, this case corresponds to this example:
>  (module
>   (global $g1 (import "foo" "bar") (mut i32))
>   (global $g2 i32 (get_global $g1)))
> I don't see why $g2 is necessarily indirect: it isn't imported or exported. 
> It seems like you need to handle both cases here.  Could you add a test case?

Initializer expressions cannot reference mutable globals at present.  This case shouldn't be here at all, I think.

I guess it's an interesting question whether initializer expressions should be able to reference mutable globals, now that we have them, but I don't see anything about that in the proposal.  The use case is more compelling than before, for sure.

Comment 32

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4aa3c7c69824742ccb8667dd52ccf5841871da8

Comment 33

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5502d567e3f5

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c82560d19d9
WebAssembly.Global object identity + mutable export. r=luke

Comment 35

[Lars T Hansen [:lth]]

This optimization remains to be done and will be spun off in a new bug:

> Instead of doing the indirection as part of the MWasm(Load|Store)GlobalVar op, which necessarily does two loads on every global read/write, could you use MWasmLoadTls to load the address of the global since MWasmLoadTls is super-GVNable.  I think this should be pretty helpful for common cases like: "STACKTOP += 4; i32.load (STACKTOP)".
> 
> Specifically, it looks like MWasm(Load|Store)GlobalVar would take either a (tls, globalDataOffset) or (address-of-global).  Changing globalDataOffset to be relative to tls, instead of (tls+offsetof(TlsData, globalArea)), would allow both cases to be collapsed into one (ptr, offset).

Also, we're discussing whether it would be OK to allow imported mutable globals to be referenced from initializers of globals in the module, but that's a longer discussion I expect, and will be handled in a new bug if/when appropriate.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05d423e63537
port jit-test changes to wpt.  r=me  CLOSED TREE

Comment 37

[Lars T Hansen [:lth]]

The patches are under threat of backout (and probably will be backed out later today) because they appear to cause Windows PGO to run out of heap space.  (I saw this on try but figured it was a buildbot blip.)

For example, https://treeherder.mozilla.org/logviewer.html#?job_id=169847478&repo=mozilla-inbound&lineNumber=37434.

There's no particular reason why these patches should cause PGO to flip out, so likely we're just perturbing something.

Comment 38

[(Away)]

(In reply to Lars T Hansen [:lth] from comment #37)

12:22:13     INFO -  z:\build\build\src\third_party\aom\aom_dsp\simd\v64_intrinsics_c.h(719) : fatal error C1002: compiler is out of heap space in pass 2

Since this is in AOM code, I suspect it is due to the large function sizes seen in bug 1412889. It is fixed upstream but our efforts to update (bug 1445683) have hit roadblocks.

If this is blocking you, we can probably just disable PGO in the affected code (AOM won't be hit in a profile anyway). You can do this by wrapping some region a file with:

#if defined(_MSC_VER)
#pragma optimize("g", off)
#endif
...
#if defined(_MSC_VER)
#pragma optimize("", on)
#endif

Or for an entire file, edit its moz.build to say:

SOURCES['foo.cpp'].no_pgo = True

(You'll need to move it from UNIFIED_SOURCES to SOURCES for this to work.)

Comment 39

[Lars T Hansen [:lth]]

Philor notes that this PGO failure is a known itermittent: bug 1445922

Comment 40

[Luke Wagner [:luke]]

Moreover, it looks like the PGO failures started a cset before this bug's and were fixed by the backout of bug 1444119.  So it looks like we're in the clear?

Comment 41

[Lars T Hansen [:lth]]

I doubt "fixed" is the word we're looking for here but yes, I think we're in the clear :)

Comment 42

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/5c82560d19d9
https://hg.mozilla.org/mozilla-central/rev/05d423e63537

Comment 43

[Ionuț Goldan [:igoldan]]

Performance win:

== Change summary for alert #12349 (as of Fri, 23 Mar 2018 08:41:03 GMT) ==

Improvements:

  3%  tp5o_webext responsiveness linux64 opt e10s stylo     1.67 -> 1.62

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12349