Closed Bug 814861 Opened 7 years ago Closed 7 years ago

IonMonkey: add type specialized paths for instanceof

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: bhackett, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (595eed63d243) (obsolete) — Splinter Review
Currently IonMonkey does not do any specialization for instanceof and always emits tests to see if that rhs is a normal function, bound function, and find its .prototype, even if all this information is statically known.  From looking at benchmarks and a couple JS frameworks (jQuery, Prototype.js) instanceof operators are on specific functions --- x instanceof Foo, rather than x instanceof y --- so this static information should usually be available.  The attached patch improves instanceof significantly, though it doesn't seem to affect benchmarks (none of which really pound on instanceof).  The main benefit of this patch is that it will allow simplification / ripping out of the existing instanceof codegen.

function blah() {}
function foo() {
  var tmp, a = new blah(), b = new Object(), res = false;
  for (var i = 0; i < 100000000; i++) {
    res = a instanceof blah;
    tmp = a;
    a = b;
    b = tmp;
  }
}
foo();

js --ion before: 528
js --ion after: 362
d8: 17290 (!)
Attachment #684850 - Flags: review?(dvander)
Comment on attachment 684850 [details] [diff] [review]
patch (595eed63d243)

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

::: js/src/ion/CodeGenerator.cpp
@@ +4192,5 @@
> +
> +    // Crawl the lhs's prototype chain to search for prototypeObject.
> +    //
> +    // output = obj->type->proto; // getTaggedProto()
> +    // while (1) {

This pseudo code does not bring anything, adding a mention of fun_hasInstance and IsDelegate would be better than this pseudo code.

@@ +4211,5 @@
> +    masm.loadPtr(Address(objReg, JSObject::offsetOfType()), output);
> +    masm.loadPtr(Address(output, offsetof(types::TypeObject, proto)), output);
> +
> +    Label loopPrototypeChain;
> +    masm.bind(&loopPrototypeChain);

nit: Add a '{' before this masm.bind and indent the content of the generated loop.

@@ +4228,5 @@
> +    // Load output->type->proto
> +    masm.loadPtr(Address(output, JSObject::offsetOfType()), output);
> +    masm.loadPtr(Address(output, offsetof(types::TypeObject, proto)), output);
> +
> +    masm.jump(&loopPrototypeChain);

nit: Add a '}' after this instruction.

@@ +4234,5 @@
> +    masm.bind(&testLazy);
> +
> +    Label lazyProto;
> +    masm.branchPtr(Assembler::Equal, output, ImmWord(1), &lazyProto);
> +    if (!bailoutFrom(&lazyProto, ins->snapshot()))

Knowing that the non-typed InstanceOf does not bail out on this case, this would be a shame that a more optimized function cannot handle cases of a generic one without providing worse performances.

Use an oolCallVM to IsDelegate instead of bailing out.

::: js/src/ion/IonBuilder.cpp
@@ +6505,5 @@
> +
> +    // If this is an 'x instanceof function' operation and we can determine the
> +    // prototype being tested for, use a typed path.
> +    RawObject rhsObject = types.rhsTypes ? types.rhsTypes->getSingleton() : NULL;
> +    if (rhsObject && rhsObject->isFunction() && !rhsObject->isBoundFunction()) {

nit: I will suggest to rewrite this block à la GetElem, except instead of adding a new function for each case, you wrap the optimization in a do-while block and use break to skip the current optimization and return in case of failure/success:

// … comment …
do {
  RawObject …;

  if (!rhsObject || !rhsObject->isFunction())
    break;

  return true;
} while (JS_NOT_REACHED("End of Typed InstanceOf"), false);


That way we can avoid increasing the indentation level.

::: js/src/ion/TypePolicy.cpp
@@ +386,5 @@
>         BoxPolicy<0>::staticAdjustInputs(def);
>      }
>  
> +    if (def->numOperands() == 2) {
> +        // Unbox second operand forcefully to an object, 

nit: Remove the trailing whitespace.
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #1)
> @@ +4234,5 @@
> > +    masm.bind(&testLazy);
> > +
> > +    Label lazyProto;
> > +    masm.branchPtr(Assembler::Equal, output, ImmWord(1), &lazyProto);
> > +    if (!bailoutFrom(&lazyProto, ins->snapshot()))
> 
> Knowing that the non-typed InstanceOf does not bail out on this case, this
> would be a shame that a more optimized function cannot handle cases of a
> generic one without providing worse performances.
> 
> Use an oolCallVM to IsDelegate instead of bailing out.

Hmm, I was thinking about this and I'm pretty sure the only cases where the lazy proto case currently can occur is for cross compartment wrappers, and since TI knows nothing about CCWs this would only trip on code like 'something-from-another-compartment instanceof something-from-this-compartment', which doesn't seem like code that someone would really write.  In the future this could also trip on proxies with scripted getProto handlers.  billm knows more I think.

Using an OOL call here has multiple costs: it is more code, practically dead and poorly tested code, and would render the instruction effectful, so that it cannot be GVN'ed or LICM'ed.  It seems to me then that this is a textbook case for where we *do* want to bailout, as not bailing out reduces quality of the generated code, and that what's missing is a simple mechanism to keep track of when and where we've bailed out so that if they become excessive at a given point we can recompile Ion code that does the generic thing.  That is a general problem with how bailouts work in Ion though, and while I don't think it would be good to fix now it is something that would be nice to visit after the baseline compiler is in shape.
Fix nits per comment 1, except the OOL call issue (see comment 2).
Attachment #684898 - Flags: review?(nicolas.b.pierron)
Attachment #684850 - Attachment is obsolete: true
Attachment #684850 - Flags: review?(dvander)
Comment on attachment 684898 [details] [diff] [review]
patch (595eed63d243)

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

::: js/src/ion/MIR.h
@@ +5312,5 @@
>      }
>  };
>  
> +// Implementation for instanceof operator with specific rhs.
> +class MInstanceOfTyped

nit: If we're going to deoptimize the other case, this should be MInstanceOf and the other case MCallInstanceOf. Same for LIR. We can do that once the other version is in place, though.
(In reply to Brian Hackett (:bhackett) from comment #2)
> (In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #1)
> > @@ +4234,5 @@
> > > +    masm.bind(&testLazy);
> > > +
> > > +    Label lazyProto;
> > > +    masm.branchPtr(Assembler::Equal, output, ImmWord(1), &lazyProto);
> > > +    if (!bailoutFrom(&lazyProto, ins->snapshot()))
> > 
> > Knowing that the non-typed InstanceOf does not bail out on this case, this
> > would be a shame that a more optimized function cannot handle cases of a
> > generic one without providing worse performances.
> > 
> > Use an oolCallVM to IsDelegate instead of bailing out.
> 
> Hmm, I was thinking about this and I'm pretty sure the only cases where the
> lazy proto case currently can occur is for cross compartment wrappers, and
> since TI knows nothing about CCWs this would only trip on code like
> 'something-from-another-compartment instanceof
> something-from-this-compartment', which doesn't seem like code that someone
> would really write.  In the future this could also trip on proxies with
> scripted getProto handlers.  billm knows more I think.
> 
> Using an OOL call here has multiple costs: it is more code, practically dead
> and poorly tested code, and would render the instruction effectful, so that
> it cannot be GVN'ed or LICM'ed.  It seems to me then that this is a textbook
> case for where we *do* want to bailout, as not bailing out reduces quality
> of the generated code, and that what's missing is a simple mechanism to keep
> track of when and where we've bailed out so that if they become excessive at
> a given point we can recompile Ion code that does the generic thing.  That
> is a general problem with how bailouts work in Ion though, and while I don't
> think it would be good to fix now it is something that would be nice to
> visit after the baseline compiler is in shape.

Guessing that the rhs is a constant, this use case show up in both Gmail and Facebook (see Bug 794947 and Bug 803730 comment 10).  Based on Bug 803730 comment 1,  I think this case should be handled even if we cannot move the instanceOf instruction.

Monitoring getProto proxy case should help us to switch from a moveable instanceOf to a non-moveable one.
Comment on attachment 684898 [details] [diff] [review]
patch (595eed63d243)

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

The patch is good, but it should either use a call VM (instead of a bailout), or land after Bug 803710 as this modification might cause a loop of bailouts (as reported by in comment 5).
Attachment #684898 - Flags: review?(nicolas.b.pierron) → review+
Pushed, with an OOL call for IsDelegate.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e390f459239e
https://hg.mozilla.org/mozilla-central/rev/e390f459239e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Since bug 814177 regressed beta performance significantly, we should either back that out or take this on beta. I think this is low risk enough - what do you think, Brian?
Comment on attachment 684898 [details] [diff] [review]
patch (595eed63d243)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Fix performance regression caused by uplift of bug 814177.
User impact if declined: Slower benchmarks
Testing completed (on m-c, etc.): On m-c since 11/26
Risk to taking this patch (and alternatives if risky): This patch is pretty low risk, but does add new functionality to IonMonkey (new codegen for the instanceof operation) so I don't know if this is appropriate for uplifting.  Alternatives are eating the performance regression (see bug 814177) or backing out bug 814177 from branches.
String or UUID changes made by this patch: None
Attachment #684898 - Flags: approval-mozilla-beta?
Attachment #684898 - Flags: approval-mozilla-aurora?
given Brian's assessment, let's go with backing out bug 814177 instead - I'd rather we not take any unnecessary risks and it's just for two more releases.
Attachment #684898 - Flags: approval-mozilla-beta?
Attachment #684898 - Flags: approval-mozilla-beta-
Attachment #684898 - Flags: approval-mozilla-aurora?
Attachment #684898 - Flags: approval-mozilla-aurora-
(In reply to Lukas Blakk [:lsblakk] from comment #11)
> given Brian's assessment, let's go with backing out bug 814177 instead - I'd
> rather we not take any unnecessary risks and it's just for two more releases.

Backing out Bug 814177 is a risk.
Comment on attachment 684898 [details] [diff] [review]
patch (595eed63d243)

talked in IRC with Nicolas and it turns out that taking this patch is the least risky option, we can't leave the 5% perf hit that bug 814177 introduces and backing it out is higher risk than taking this patch.  Approving for uplift given those options.
Attachment #684898 - Flags: approval-mozilla-beta-
Attachment #684898 - Flags: approval-mozilla-beta+
Attachment #684898 - Flags: approval-mozilla-aurora-
Attachment #684898 - Flags: approval-mozilla-aurora+
TM is for when it hit m-c.
Target Milestone: mozilla18 → mozilla20
You need to log in before you can comment on or make changes to this bug.