856829 - BaselineCompiler: Add optimized stubs for GetProp-getter and SetProp-setter calling JSNative targets

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Needed for DOM access performance.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Add getter/setter IC stubs for JSNatives

Ion didn't handle JSNative setters before this, so this patch adds a new exit frame type for JSNative setters.

The ICGetProp_CallScripted and ICSetProp_CallScripted stubs are refactored out into shared base stubs: ICGetPropCallGetter and ICSetPropCallSetter, which are inherited by both CallScripted and CallNative stubs.

The compilers too uses a shared base class.  I didn't use the same compiler for both because the register allocation is different enough, and there wasn't enough common code, to seem to warrant it.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 732469 [details] [diff] [review]
Add getter/setter IC stubs for JSNatives

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

Looks great! I think it'd be good to take another look after addressing the frame descriptor comment though.

::: js/src/ion/BaselineIC.cpp
@@ +3083,5 @@
> +
> +    if (shape->hasSlot() || shape->hasDefaultGetter())
> +        return false;
> +
> +    if (!shape->hasGetterValue() || !shape->getterObject()->isFunction())

The Ion IC checks shape->getterValue().isObject(), why is it not necessary here?

@@ +3207,5 @@
> +
> +    if (shape->hasSlot() || shape->hasDefaultSetter())
> +        return false;
> +
> +    if (!shape->hasSetterValue() || !shape->setterObject()->isFunction())

Same here.

@@ +5260,5 @@
> +
> +    // The exit frame format will re-use Ion's IonOOLNativeGetterExitFrameLayout,
> +    // which expects a pointer to the IonCode for the stub, so that it may be rooted.
> +    // Baseline does that implicitly, so we can just store a null here for that.
> +    masm.Push(ImmWord((void *) NULL));

Shouldn't the marking code in IonFrames check for a NULL IonCode *?

@@ +5297,5 @@
> +    masm.movePtr(BaselineStackReg, vpReg);
> +    masm.move32(Imm32(0), argcReg);
> +    masm.loadJSContext(contextReg);
> +
> +    if (!masm.buildOOLFakeExitFrame(NULL))

This will push an OptimizedJS frame descriptor. I think we want to use EmitCreateStubFrameDescriptor, like the Call_Native stub. It would be good to temporarily add a JS_GC call or StackIter to the JSNative getter in the shell to test the frame iteration code.

::: js/src/ion/arm/BaselineHelpers-arm.h
@@ +122,5 @@
>  
>  // Size of vales pushed by EmitEnterStubFrame.
>  static const uint32_t STUB_FRAME_SIZE = 4 * sizeof(void *);
>  static const uint32_t STUB_FRAME_SAVED_STUB_OFFSET = sizeof(void *);
> +static const uint32_t STUB_FRAME_PUSHED = 2 * sizeof(void *);

Nit: a short comment here would be good (and x86/x64)

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: Add getter/setter IC stubs for JSNatives, try 2

> The Ion IC checks shape->getterValue().isObject(), why is it not necessary
> here?

Good point.  It's necessary here.  Also, I refactored IsCacheableGetterCallScripted and IsCacheableGetterCallNative into a single function (likewise for setters).

> Shouldn't the marking code in IonFrames check for a NULL IonCode *?
> 
> This will push an OptimizedJS frame descriptor. I think we want to use
> EmitCreateStubFrameDescriptor, like the Call_Native stub. It would be good
> to temporarily add a JS_GC call or StackIter to the JSNative getter in the
> shell to test the frame iteration code.

Good catch.  Fixed this up to use a VMcall instead, no changes to ion code necessary.

> > +static const uint32_t STUB_FRAME_PUSHED = 2 * sizeof(void *);
> 
> Nit: a short comment here would be good (and x86/x64)

STUB_FRAME_PUSHED is no longer necessary either.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 732654 [details] [diff] [review]
Add getter/setter IC stubs for JSNatives, try 2

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

Great idea to use CallVM.

Comment 5

[Kannan Vijayan [:djvj]]

Pushed: https://hg.mozilla.org/projects/ionmonkey/rev/d8b068c9dbc1