IonMonkey: Inline call to common getters and setters.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: efaust)

Tracking

Other Branch
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

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

Updated

5 years ago
Assignee: general → efaust
Status: NEW → ASSIGNED
Assignee: efaust → general
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Assignee: general → efaust
(Assignee)

Comment 1

5 years ago
Created attachment 628461 [details] [diff] [review]
Inline common getters.
Attachment #628461 - Flags: review?(nicolas.b.pierron)
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.
Attachment #628461 - Flags: review?(nicolas.b.pierron)
> 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).
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).
(Assignee)

Comment 5

5 years ago
Indeed, the current patch will not even trigger on this test case.
OK, then I clearly misunderstood what this bug is about.  ;)  When _does_ it trigger?
(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.
(Assignee)

Updated

5 years ago
Attachment #628461 - Attachment is obsolete: true
(Assignee)

Comment 8

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

Comment 9

5 years ago
Created attachment 630663 [details] [diff] [review]
Inline common getters. Now includes prototypal properties.
(Assignee)

Comment 10

5 years ago
Created attachment 630664 [details]
Simple Micro Benchmark

Runtimes without optimization run about 980ms.
Runtimes with optimization run about 10ms.
(Assignee)

Updated

5 years ago
Attachment #630663 - Flags: review?(dvander)
Attachment #630663 - Flags: review?(bhackett1024)
(Assignee)

Comment 11

5 years ago
Created attachment 630828 [details] [diff] [review]
Inline common setters as well.
Attachment #630828 - Flags: review?(dvander)
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).
Attachment #630663 - Flags: review?(bhackett1024)
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
Attachment #630663 - Flags: review?(dvander) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 631518 [details] [diff] [review]
Stop marking new DOM objects as having no TI.
(Assignee)

Comment 15

5 years ago
Created attachment 632048 [details] [diff] [review]
Inline common gett calls. Now with correctly protected prototypal getters as well.
Attachment #630663 - Attachment is obsolete: true
Attachment #632048 - Flags: review?(bhackett1024)
(Assignee)

Updated

5 years ago
Attachment #632048 - Flags: review?(bhackett1024)
(Assignee)

Comment 16

5 years ago
Created attachment 632095 [details] [diff] [review]
Inline common getters. Now includes safe prototypal getters.
Attachment #632048 - Attachment is obsolete: true
Attachment #632095 - Flags: review?(bhackett1024)
(Assignee)

Updated

5 years ago
Depends on: 764148
(Assignee)

Updated

5 years ago
Attachment #630828 - Attachment is obsolete: true
Attachment #630828 - Flags: review?(dvander)
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
Attachment #632095 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 18

5 years ago
Created attachment 633356 [details] [diff] [review]
Inline common setters.
Attachment #633356 - Flags: review?(dvander)
Attachment #633356 - Flags: feedback+
(Assignee)

Updated

5 years ago
Depends on: 765454
Comment on attachment 633356 [details] [diff] [review]
Inline common setters.

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

Nice.
Attachment #633356 - Flags: review?(dvander) → review+
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
(Assignee)

Comment 21

5 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.