Note: There are a few cases of duplicates in user autocompletion which are being worked on.

IonMonkey: Optimize JSOP_FUNCALL

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dvander, Assigned: sstangl)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Needed for deltablue and earley-boyer, I think. Ideally we want:

    f.call(x, ...)

To become:

    f(...)   ; where this = x

We should be able to inline through in the best case (I think JM+TI does this), and for other cases fall back to a transformation to MCall.
(Assignee)

Updated

5 years ago
Assignee: general → sstangl
(Assignee)

Comment 1

5 years ago
We already support JSOP_FUNCALL as described. The hard case for JSOP_THIS isn't handled, but that is a separate bug.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 2

5 years ago
I've been tricked. Dvander pointed out that we don't really handle JSOP_FUNCALL at all -- we go through a native. Reopening.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 3

5 years ago
Created attachment 609518 [details] [diff] [review]
JSOP_FUNCALL without inlining.

This implements JSOP_FUNCALL optimizations without inlining. The normal call mechanism is reused by shifting the slots down by one, overwriting the native, and unwrapping the MPassArg(JSFunction *).

Unfortunately, logic to inline functions queries TI for safety/argument information, and with the transformation of f.call(...) -> f(...), the types returned by TI no longer correspond to the MIR being built.

The obvious suggestion is to collect TI information before shimmying the slots down, but native inlining needs access to arbitrary many TypeSets. Another option is to have poppedTypes() check whether pc == JSOP_FUNCALL in certain situations, and if so, actually check one slot higher. Doesn't sound pleasant.
(Assignee)

Comment 4

5 years ago
Comment on attachment 609518 [details] [diff] [review]
JSOP_FUNCALL without inlining.

JM doesn't inline, so it's probably not awful for us to also not inline.

v8-v6 deltablue goes ~500ms -> ~480ms on local x86; all other tests unaffected.
Attachment #609518 - Flags: review?(dvander)
(Reporter)

Comment 5

5 years ago
Comment on attachment 609518 [details] [diff] [review]
JSOP_FUNCALL without inlining.

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

::: js/src/ion/IonBuilder.cpp
@@ +125,5 @@
>      return state;
>  }
>  
> +JSObject *
> +IonBuilder::getSingleObject(types::TypeSet *types)

nit: This looks like it could be a static, instead of a member function (maybe not even declared in IonBuilder.h)

@@ +129,5 @@
> +IonBuilder::getSingleObject(types::TypeSet *types)
> +{
> +    if (!types || types->unknownObject() || types->getObjectCount() != 1)
> +        return NULL;
> +    return types->getSingleObject(0);

I think this can just be:
    if (!types)
        return NULL;
    return types->getSingleton();

@@ +2566,5 @@
> +    if (!native || !native->isNative() || native->native() != &js_fun_call)
> +        return makeCall(native, argc, false);
> +
> +    // Extract call target.
> +    JSObject *funobj   = getSingleObject(oracle->getCallArg(script, argc, 0, pc));

Are we guaranteed a non-null funobj here?

::: js/src/ion/MIRGraph.h
@@ +142,5 @@
>      // not be used under most circumstances.
>      void rewriteSlot(uint32 slot, MDefinition *ins);
>  
> +    // Rewrites a slot based on its current depth (same as argument to peek()).
> +    void rewriteDepth(int32 depth, MDefinition *ins);

Maybe "rewriteAtDepth" would be a better name.
Attachment #609518 - Flags: review?(dvander) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/8a2010ae3d08
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 755194

Updated

5 years ago
Depends on: 755832

Updated

5 years ago
Depends on: 776359
You need to log in before you can comment on or make changes to this bug.