870821 - IonMonkey: Avoid type barriers on integer CALLELEM ops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Indirect calls in asm.js are implemented appear in the form of a[i]().  I've seen similar patterns on hand written apps in the past, some emulator I think.  Having type barriers on these accesses isn't that useful --- the barriers add significant overhead if there are many callees, can cause invalidations, and the gain in precision is useless if there is more than one callee.  The one callee use case doesn't seem that compelling, and the speedup there should be marginal anyhow.

The attached patch tries to avoid barriers on these accesses by eagerly populating the observed types with objects which the access could actually read.  It also does a bit of cleanup to remove redundant / not robust code when fetching types and singleton objects from type sets.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 747997 [details] [diff] [review]
patch

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

::: js/src/ion/MIR.cpp
@@ +2520,5 @@
> +
> +    JS_ASSERT(observed->noConstraints());
> +
> +    types::StackTypeSet *types = obj->resultTypeSet();
> +    if (!types || !types->unknownObject()) {

No "!" before types->unknownObject()?

::: js/src/jsinferinlines.h
@@ +1555,5 @@
> +{
> +    JS_ASSERT(cx->compartment->activeAnalysis);
> +    TypeObject *type = getTypeObject(i);
> +    if (!type) {
> +        JSObject *singleton = getSingleObject(i);

Since getObjectCount overapproximates, we need

if (!singleton)
    return NULL;

here, right? That's also what the callers seem to expect.

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5812c4bef74

Comment 3

[Brian Hackett [Laid off!]]

I think this made octane-gameboy about 13% happier.

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b5812c4bef74

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 747997 [details] [diff] [review]
patch

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

::: js/src/ion/MIR.cpp
@@ +2537,5 @@
> +            observed->addType(cx, types::Type::AnyObjectType());
> +            return;
> +        }
> +
> +        types::HeapTypeSet *property = object->getProperty(cx, JSID_VOID, false);

Should this line of code should be using the `id` defined on line 2529, rather than JSID_VOID?

Otherwise that variable is unused (which causes a warning in clang, which is why I noticed it).

Comment 6

[Brian Hackett [Laid off!]]

Good catch, yes, that should be using |id| instead (though in current uses |id| will always be JSID_VOID anyways).

https://hg.mozilla.org/integration/mozilla-inbound/rev/715278d3d2da

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/715278d3d2da