845948 - BaselineCompiler: Add optimized stubs for GETPROP and SETPROP invocations resulting in getter/setter calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Box2d is hitting this a lot, as it uses getters and setters extensively.  This will eliminate about 350k fallback hits and slow vm calls from baseline.

Comment 1

[Jan de Mooij [:jandem]]

Do we know why we are spending so much time in baseline? If we could run these functions in Ion it could even inline the getters/setters...

Comment 2

[Kannan Vijayan [:djvj]]

No, not sure.  We're bailing out more frequently than with trunk, but I haven't looked closely into whether that's just legit type instability or some other issue.

However this part is easily fixable and the spew noise from it is making it more troublesome to look into what other issues there might be.

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: Optimized stubs to call getters and setters

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 719599 [details] [diff] [review]
Optimized stubs to call getters and setters

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

Looks good, though you might want to ask jandem about the call IC stubs themselves, I don't know the calling code at all.

::: js/src/ion/BaselineIC.cpp
@@ +2383,5 @@
>                         : obj->dynamicSlotIndex(slot) * sizeof(Value);
>  }
>  
>  static bool
> +IsCacheableProtoChain(RawObject obj, RawObject holder)

These RawObject changes aren't necessary, RawObject itself will be going away pretty soon hopefully.

::: js/src/ion/arm/BaselineHelpers-arm.h
@@ +195,5 @@
>      }
>  }
>  
>  inline void
> +EmitUnstowICValues(MacroAssembler &masm, int values, bool discard=false)

What is this change for (and those for other architectures)?  The new calls to this function do not pass an explicit 'discard' argument.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 719599 [details] [diff] [review]
Optimized stubs to call getters and setters

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

Looks good. I don't know if we have good tests for scripted getters/setters, so would be good to add tests for:

(1) Calling a getter/setter with a bunch of formal arguments, to test the arguments rectifier path.
(2) Make sure arguments.length is correct inside the getter/setter.
(3) Make sure the getter/setter is passed the right arguments, |this| (and |undefined| for extra formals).

Comment 6

[Kannan Vijayan [:djvj]]

(In reply to Brian Hackett (:bhackett) from comment #4)
> These RawObject changes aren't necessary, RawObject itself will be going
> away pretty soon hopefully.

Gotcha.

> What is this change for (and those for other architectures)?  The new calls
> to this function do not pass an explicit 'discard' argument.

I thought I needed it, but it turned out I didn't.  Forgot to reverse those changes.  Fixed that up.

Comment 7

[Kannan Vijayan [:djvj]]

> (1) Calling a getter/setter with a bunch of formal arguments, to test the
> arguments rectifier path.
> (2) Make sure arguments.length is correct inside the getter/setter.
> (3) Make sure the getter/setter is passed the right arguments, |this| (and
> |undefined| for extra formals).

Good suggestion.  Done.

Pushed:
https://hg.mozilla.org/projects/ionmonkey/rev/60398cac8cd6

Comment 8

[Kannan Vijayan [:djvj]]

Waiting for TBPL green until closing.

Comment 9

[Kannan Vijayan [:djvj]]

Backed out to stabilize oranges on tbpl: https://hg.mozilla.org/projects/ionmonkey/rev/f3306a47ed30

Comment 10

[Kannan Vijayan [:djvj]]

Pushed: https://hg.mozilla.org/projects/ionmonkey/rev/5573013369bc
Watching tbpl before closing.