848122 - BaselineCompiler: Generalize CallScripted stubs once callee-specific stubs get too numerous

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Seeing this hit baseline a lot in the main opcode dispatch function in the gbemu code.

The dispatch is done by looking up an opcode-specific handler function from an array, and calling that in 'GameBoyCore.prototype.executeIteration':

    this.OPCODE[opcodeToExecute](this);

We quickly fill up our chain with CallScripted stubs and then continually hit the fallback.  This hits about 30k times with parallel compilation on.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Patch

This provides infrastructure for generalizing ICCall_Scripted to ICCall_AnyScripted.

It uses extra_ on the fallback stub to keep track of the number of optimized scripted and native call stubs attached, and when those get too high it replaces them with a generalized stub.

This is only done for Call_Scripted => Call_AnyScripted, but when we get around to it, generalizing Call_Native => Call_AnyNative should be pretty straightforward.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 722548 [details] [diff] [review]
Patch

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

::: js/src/ion/BaselineIC.cpp
@@ +5803,5 @@
> +    {
> +        Register scratch = regs.getAny();
> +        masm.load16ZeroExtend(Address(callee, offsetof(JSFunction, flags)), scratch);
> +        masm.branchTest32(Assembler::Zero, scratch, Imm32(JSFunction::INTERPRETED), &failure);
> +    }

I think this is the only place where this stub differs from ICCall_Scripted (guard on a specific callee vs any scripted-callee). The rest of the call path is pretty complicated, would be good to share it instead of duplicating it. Maybe have a single CallScriptedCompiler to emit both?

::: js/src/ion/BaselineIC.h
@@ +4372,5 @@
> +    static const unsigned NATIVE_COUNT_SHIFT = 4;
> +
> +    static const uint32_t MAX_OPTIMIZED_STUBS = 16;
> +    static const uint32_t MAX_SCRIPTED_STUBS = 7;
> +    static const uint32_t MAX_NATIVE_STUBS = 7;

Instead of keeping track of the state here I think it would be easier and less error-prone to check the stub count: if it's < the threshold attach a specialized stub for the callee, else replace any Scripted stubs and add a Call_AnyScripted stub if there's none.

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: Patch v2

Comments addressed.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 722865 [details] [diff] [review]
Patch v2

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

Awesome, r=me with nits addressed.

::: js/src/ion/BaselineIC.cpp
@@ +5393,5 @@
> +
> +        // Generalied native call stubs are not here yet!
> +        JS_ASSERT(!stub->nativeStubsAreGeneralized());
> +
> +        if (stub->nativeStubCount() >= ICCall_Fallback::MAX_SCRIPTED_STUBS) {

Nit: MAX_NATIVE_STUBS

@@ +5633,5 @@
> +        masm.branchPtr(Assembler::NotEqual, expectedScript, callee, &failure);
> +    } else {
> +        Register scratch = regs.getAny();
> +        masm.load16ZeroExtend(Address(callee, offsetof(JSFunction, flags)), scratch);
> +        masm.branchTest32(Assembler::Zero, scratch, Imm32(JSFunction::INTERPRETED), &failure);

masm.branchIfFunctionHasNoScript(callee, &failure);

::: js/src/ion/BaselineIC.h
@@ +4380,5 @@
> +    static const unsigned CONSTRUCTING_FLAG = 0x100;
> +
> +    static const uint32_t MAX_OPTIMIZED_STUBS = 16;
> +    static const uint32_t MAX_SCRIPTED_STUBS = 7;
> +    static const uint32_t MAX_NATIVE_STUBS = 7;

It should be fine to just have a single MAX_OPTIMIZED_STUBS I think (mixed native/scripted sites are rare).

@@ +4522,3 @@
>              return ICCall_Scripted::New(space, getStubCode(), firstMonitorStub_, calleeScript_);
> +        else
> +            return ICCall_AnyScripted::New(space, getStubCode(), firstMonitorStub_);

Nit: "no else after return" style-rule.

Comment 5

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #4) 
> ::: js/src/ion/BaselineIC.h
> @@ +4380,5 @@
> > +    static const unsigned CONSTRUCTING_FLAG = 0x100;
> > +
> > +    static const uint32_t MAX_OPTIMIZED_STUBS = 16;
> > +    static const uint32_t MAX_SCRIPTED_STUBS = 7;
> > +    static const uint32_t MAX_NATIVE_STUBS = 7;
> 
> It should be fine to just have a single MAX_OPTIMIZED_STUBS I think (mixed
> native/scripted sites are rare).

As discussed on IRC, leaving this as is.

Otherise, comments addressed and pushed:
https://hg.mozilla.org/projects/ionmonkey/rev/9a2dab65c85d

Waiting for tbpl green before closing.