IonMonkey: Avoid type barriers on integer CALLELEM ops

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

(Blocks: 1 bug)

Other Branch
mozilla24
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 747997 [details] [diff] [review]
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.
Attachment #747997 - Flags: review?(jdemooij)
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.
Attachment #747997 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 3

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

Comment 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/b5812c4bef74
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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).
(Reporter)

Comment 6

5 years ago
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
You need to log in before you can comment on or make changes to this bug.