Last Comment Bug 757932 - IonMonkey: Inline call to common getters and setters.
: IonMonkey: Inline call to common getters and setters.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Eric Faust [:efaust]
:
Mentors:
Depends on: 764148 765454
Blocks: 747288
  Show dependency treegraph
 
Reported: 2012-05-23 11:34 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-06-25 16:11 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Inline common getters. (14.04 KB, patch)
2012-05-30 13:53 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review
Inline common getters. Now includes prototypal properties. (20.63 KB, patch)
2012-06-06 12:13 PDT, Eric Faust [:efaust]
dvander: review+
Details | Diff | Splinter Review
Simple Micro Benchmark (409 bytes, application/x-javascript)
2012-06-06 12:15 PDT, Eric Faust [:efaust]
no flags Details
Inline common setters as well. (8.80 KB, patch)
2012-06-06 20:52 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review
Stop marking new DOM objects as having no TI. (24.17 KB, patch)
2012-06-08 13:45 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review
Inline common gett calls. Now with correctly protected prototypal getters as well. (22.91 KB, patch)
2012-06-11 15:56 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review
Inline common getters. Now includes safe prototypal getters. (22.88 KB, patch)
2012-06-11 18:06 PDT, Eric Faust [:efaust]
bhackett1024: review+
Details | Diff | Splinter Review
Inline common setters. (8.69 KB, patch)
2012-06-14 18:38 PDT, Eric Faust [:efaust]
dvander: review+
nicolas.b.pierron: feedback+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-05-23 11:34:12 PDT
Currently IonMonkey look to optimize cases where we have a common result and a common fixed slot, and is doing Inline Caches otherwise.  DOM classes can have inheritance rules and are sharing properties.  These properties are using getters and setters to manipulate the C++ fields of the class.

This bug focus on the type objects returned by the type inference and determine if for one property, all objects are sharing the same getter and/or setter.  Then this can be inlined by calling the getter / setter of the property instead of falling back to inline caches for DOM properties.
Comment 1 Eric Faust [:efaust] 2012-05-30 13:53:56 PDT
Created attachment 628461 [details] [diff] [review]
Inline common getters.
Comment 2 Nicolas B. Pierron [:nbp] 2012-05-30 15:12:10 PDT
Comment on attachment 628461 [details] [diff] [review]
Inline common getters.

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

Fix the style issue, and ask bhackett or jandem to review in details TestCommonGetter.
Move the BarrierArgs structure to the Oracle, and if possible get a few function of the oracle return the BarrierArgs structure.

Otherwise, it sounds good to me.

::: .gitignore
@@ +43,5 @@
>  
>  # Ignore the files and directory that Eclipse IDE creates
>  .project
>  .cproject
>  .settings/

Without the .gitignore ;)

::: js/src/ion/IonBuilder.cpp
@@ +3003,5 @@
>      }
>  
> +    if(!barrier) {
> +        return makeCall(target, argc, constructing);
> +    }

nit: no braces for one line if.

@@ +3085,5 @@
>  
> +    JS_ASSERT(barrier);
> +    if(barrier->pushBarrier) {
> +        return pushTypeBarrier(call, barrier->types, barrier->barrier);
> +    }

nit: remove braces.

Add a pushTypeBarrier function which accept an struct BarrierArgs * as argument, and you might even want to get the BarrierArgs directly from the oracle.

@@ +3097,5 @@
>      types::TypeSet *barrier;
>      types::TypeSet *types = oracle->returnTypeSet(script, pc, &barrier);
> +    struct BarrierArgs args = {.types= types, 
> +                               .barrier = barrier, 
> +                               .pushBarrier = true};

This is a C99 extension, I am not sure it has been ported to all C++ compilers.

@@ +4368,5 @@
>  
> +
> +static inline bool
> +TestCommonGetter(JSContext *cx, types::TypeSet *types, HandleId id, 
> +                 JSFunction ** getterp)

This function lacks comments.

@@ +4370,5 @@
> +static inline bool
> +TestCommonGetter(JSContext *cx, types::TypeSet *types, HandleId id, 
> +                 JSFunction ** getterp)
> +{
> +    JSObject * getter = NULL;

JSObject *getter, and identically after.

@@ +4404,5 @@
> +    }
> +
> +    *getterp = (JSFunction *)getter;
> +    
> +    types->addFreeze(cx);

We use the oracle to abstract this kind of things.  We want to be able to replace the type inference by a more clever one later.  In addition, TI is likely to ave done so already once you requested the TypeSet.

@@ +4455,5 @@
>          return pushTypeBarrier(fixed, types, barrier);
>      }
>  
> +    // Attempt to inline common property getter. At least patch to call instead.
> +    JSFunction * commonGetter;

nit: no space between “*” and  “commonGetter”.

::: js/src/ion/IonBuilder.h
@@ +373,5 @@
>      bool makeInliningDecision(JSFunction *target);
>  
> +
> +    struct BarrierArgs {
> +        types::TypeSet *types;

I think you should move this data structure to the oracle and make a few function initializing this data structure.  Almost like UnaryTypes.

@@ +375,5 @@
> +
> +    struct BarrierArgs {
> +        types::TypeSet *types;
> +        types::TypeSet *barrier;
> +        bool pushBarrier;

pushBarrier is always set to true, what is it used for ?

::: js/src/jsinfer.h
@@ +507,5 @@
>      {}
>  };
>  
> +/* Is this a reasonable PC to be doing inlining on? */
> +inline bool isInlinePC(jsbytecode *pc);

isInlinableCall instead of isInlinePC ?

::: js/src/jsinferinlines.h
@@ +162,5 @@
> +/* Assert code to know which PCs are reasonable to be considering inlining on */
> +inline bool
> +isInlinePC(jsbytecode *pc)
> +{
> +    JS_ASSERT(pc);

We do not assert to prevent SEGV.  because both will fail after all.

@@ +166,5 @@
> +    JS_ASSERT(pc);
> +    
> +    JSOp op = JSOp(*pc);
> +
> +    if(op == JSOP_CALL ||

if␣(…

@@ +168,5 @@
> +    JSOp op = JSOp(*pc);
> +
> +    if(op == JSOP_CALL ||
> +       op == JSOP_FUNCALL ||
> +       op == JSOP_FUNAPPLY ||

sstangl: Don't we inline JSOP_NEW too ?

@@ +170,5 @@
> +    if(op == JSOP_CALL ||
> +       op == JSOP_FUNCALL ||
> +       op == JSOP_FUNAPPLY ||
> +       op == JSOP_GETPROP ||
> +       op == JSOP_CALLPROP ||

Comment why GetPorp and CallProp are good inlinable calls.
Comment 3 Boris Zbarsky [:bz] (TPAC) 2012-05-30 20:21:14 PDT
> This is a C99 extension, I am not sure it has been ported to all C++ compilers.

In particular, this patch does not compile in our default Mac configuration (GCC 4.2).
Comment 4 Boris Zbarsky [:bz] (TPAC) 2012-05-30 20:44:39 PDT
For what it's worth, I just tried this testcase:

  <script>
    var xhr = new XMLHttpRequest();
    var count = 10000000;
    var start = new Date();
    for (var i = 0; i < count; ++i)
      xhr.readyState;
    var end = new Date();
    alert(end - start);
  </script>

in an Ion build.  It's about 25x slower than a current trunk build.  The patch in this bug doesn't seem to move the numbers (though I believe that's expected).
Comment 5 Eric Faust [:efaust] 2012-05-31 00:26:24 PDT
Indeed, the current patch will not even trigger on this test case.
Comment 6 Boris Zbarsky [:bz] (TPAC) 2012-05-31 06:41:33 PDT
OK, then I clearly misunderstood what this bug is about.  ;)  When _does_ it trigger?
Comment 7 Nicolas B. Pierron [:nbp] 2012-05-31 09:43:47 PDT
(In reply to Eric Faust from comment #5)
> Indeed, the current patch will not even trigger on this test case.

(In reply to Boris Zbarsky (:bz) from comment #6)
> OK, then I clearly misunderstood what this bug is about.  ;)  When _does_ it
> trigger?

(patch vs. bug)  The bug is supposed to improve this test case by removing the lookup to readyState and inlining the call the the native function used as getter.

As Eric told me yesterday, this patch is apparently failing because it is pushing  an UndefinedValue() instead of pushing the ObjectValue(getter) expected by jsop_call_fun_barrier.  Eric is still working on it.
Comment 8 Eric Faust [:efaust] 2012-05-31 20:10:36 PDT
(In reply to Boris Zbarsky (:bz) from comment #6)
> OK, then I clearly misunderstood what this bug is about.  ;)  When _does_ it
> trigger?

Well, it's slightly more complicated than Nicolas would let on. The problem is that currently we only find common getters on objects of singleton type (which denies the use case where |new| is invoked, which is remarkably silly).

It remains an open question as to whether I can make it work in a more general case. I would like to, because as written, it basically never applies. I spoke with Brian last week about the possibility of basing off the prototype, but the possibility that an object could shadow the custom getter of its prototype has me worried. Perhaps TI can be made to know if any object of the type provided has shadowed a given Property?
Comment 9 Eric Faust [:efaust] 2012-06-06 12:13:48 PDT
Created attachment 630663 [details] [diff] [review]
Inline common getters. Now includes prototypal properties.
Comment 10 Eric Faust [:efaust] 2012-06-06 12:15:10 PDT
Created attachment 630664 [details]
Simple Micro Benchmark

Runtimes without optimization run about 980ms.
Runtimes with optimization run about 10ms.
Comment 11 Eric Faust [:efaust] 2012-06-06 20:52:24 PDT
Created attachment 630828 [details] [diff] [review]
Inline common setters as well.
Comment 12 Brian Hackett (:bhackett) 2012-06-07 14:21:13 PDT
Comment on attachment 630663 [details] [diff] [review]
Inline common getters. Now includes prototypal properties.

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

Looks good, but needs another pass for the stuff below.

::: js/src/ion/IonBuilder.cpp
@@ +4381,5 @@
> +
> +            // If we get nothing back, then getObjectCount() has overstepped, as
> +            // per jsinfer.h, and we are done.
> +            if (!typeObj)
> +                break;

This should be a continue instead of a break, there can be NULLs in the middle of iteration.  (jsinfer.h should really have an example.)

@@ +4393,5 @@
> +            while (walker) {
> +                if (walker->getClass()->ops.lookupProperty)
> +                    return true;
> +                walker = walker->getProto();
> +            }

This walk should be moved out of the '!curObj' if statement, as you can hit lookupProperty hooks if types->getSingleObject(i) != NULL

@@ +4432,5 @@
> +        return true;
> +
> +    *funcp = (JSFunction *)found;
> +
> +    types->addFreeze(cx);

There are more type sets that need to be frozen.  This freeze just ensures that the set of singleton/type objects being accessed remains the same, but the property being accessed could change and invalidate the testing being done here.

Unfortunately, there isn't an easy fix for this.  TI property type info does not precisely model properties with getters or setters attached --- the property just gets marked as 'configured' and has unknown type.  Fixing this directly would be difficult I think, as there isn't a good way to express getter/setter properties in type info.

The patch can still be fixed though, in a couple steps:

1. Add a shape guard on the 'unused' object above which bails out if it fails.  You can check that the 'unused' object is the same for all objects in the type set, then only one guard is required.  If the property is ever removed or reconfigured from 'unused', the guard will trip and the bailout will be taken.

2. Add freeze constraints to the property type sets of the singletons and type objects leading up the prototype chain to 'unused', which statically guard that the named property is not added to any other objects lower down on the prototype chain.  Use typeObject->getProperty(cx, id, false)->isOwnProperty(cx, typeObject, false) to test this (with NULL checks on getProperty).
Comment 13 David Anderson [:dvander] 2012-06-08 12:12:36 PDT
Comment on attachment 630663 [details] [diff] [review]
Inline common getters. Now includes prototypal properties.

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

Nice work, patch looks good from an IonMonkey standpoint but need bhackett's r+ for TI integration.

::: js/src/jsinfer.h
@@ +510,5 @@
>      {}
>  };
>  
> +/* Is this a reasonable PC to be doing inlining on? */
> +inline bool isInlinableCall(jsbytecode *pc);

nit: IsInlinableCall (statics get first letter capitalized)

::: js/src/jsinferinlines.h
@@ +164,5 @@
> +isInlinableCall(jsbytecode *pc)
> +{
> +    JSOp op = JSOp(*pc);
> +
> +	// CALL, FUNCALL, FUNAPPLY (Standard callsites)

nit: accidental hard tab

::: js/src/jsopcode.h
@@ +486,5 @@
>             op != JSOP_GOTO && op != JSOP_RETSUB;
>  }
>  
> +inline bool
> +isGetterPC(jsbytecode *pc)

nit: IsGetterPC
Comment 14 Eric Faust [:efaust] 2012-06-08 13:45:20 PDT
Created attachment 631518 [details] [diff] [review]
Stop marking new DOM objects as having no TI.
Comment 15 Eric Faust [:efaust] 2012-06-11 15:56:08 PDT
Created attachment 632048 [details] [diff] [review]
Inline common gett calls. Now with correctly protected prototypal getters as well.
Comment 16 Eric Faust [:efaust] 2012-06-11 18:06:05 PDT
Created attachment 632095 [details] [diff] [review]
Inline common getters. Now includes safe prototypal getters.
Comment 17 Brian Hackett (:bhackett) 2012-06-14 15:37:36 PDT
Comment on attachment 632095 [details] [diff] [review]
Inline common getters. Now includes safe prototypal getters.

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

Looks great!

::: js/src/ion/IonBuilder.cpp
@@ +4381,5 @@
> +        if (!curObj) {
> +            types::TypeObject *typeObj = types->getTypeObject(i);
> +
> +            // If we get nothing back, then getObjectCount() has overstepped, as
> +            // per jsinfer.h, and we are done.

Inaccurate comment, just remove
Comment 18 Eric Faust [:efaust] 2012-06-14 18:38:01 PDT
Created attachment 633356 [details] [diff] [review]
Inline common setters.
Comment 19 David Anderson [:dvander] 2012-06-18 15:46:32 PDT
Comment on attachment 633356 [details] [diff] [review]
Inline common setters.

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

Nice.
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2012-06-19 05:37:14 PDT
Comment on attachment 633356 [details] [diff] [review]
Inline common setters.

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

::: js/src/ion/IonFrames.cpp
@@ +941,3 @@
>          if (IsGetterPC(parent.pc()))
>              return 0;
> +        else if (IsSetterPC(parent.pc()))

Just if, please.

@@ +971,3 @@
>          if (IsGetterPC(inlinedParent.pc()))
>              return 0;
> +        else if (IsSetterPC(inlinedParent.pc()))

Same
Comment 21 Eric Faust [:efaust] 2012-06-25 16:11:06 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/8862e62cd1f5

Landed. Setter call inlining will take effect as soon as bug 764148 lands late today and is subsequently merged to ion.

Note You need to log in before you can comment on or make changes to this bug.