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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: djvj, Assigned: djvj)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: general → kvijayan
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...
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 719599 [details] [diff] [review]
Optimized stubs to call getters and setters
Attachment #719599 - Flags: review?(bhackett1024)
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.
Attachment #719599 - Flags: review?(bhackett1024) → review+
(Assignee)

Updated

5 years ago
Attachment #719599 - Flags: review+ → review?(jdemooij)
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).
Attachment #719599 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
> (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
(Assignee)

Comment 8

5 years ago
Waiting for TBPL green until closing.
(Assignee)

Comment 9

5 years ago
Backed out to stabilize oranges on tbpl: https://hg.mozilla.org/projects/ionmonkey/rev/f3306a47ed30
(Assignee)

Comment 10

5 years ago
Pushed: https://hg.mozilla.org/projects/ionmonkey/rev/5573013369bc
Watching tbpl before closing.
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.