1113677 - Fold instanceof based on type information

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

This is the TI-based folding mentioned in bug 1113643 comment 0.

It also detects the case where LHS is either an object or primitive but if it's an object it's known to be instanceof RHS. In this case we can't fold it completely but we can replace it with a much faster IsObject check.

This is apparently a pretty common case on Octane earley-boyer, patch wins at least a few hundred points there.

Comment 1

[Jan de Mooij [:jandem]]

Brian, does HasOnProtoChain have to do anything special to guard against mutating __proto__? I don't think so because we have similar loops elsewhere in IonBuilder, but maybe I'm missing something.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 8539293 [details] [diff] [review]
Patch

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

::: js/src/jit/IonBuilder.cpp
@@ +11162,5 @@
>      return true;
>  }
>  
> +static bool
> +HasOnProtoChain(types::TypeObjectKey *object, JSObject *protoObject, bool *hasOnProto)

This function does need to do something to make sure the prototype chain doesn't mutate.  This generally happens implicitly; when an object's prototype is mutated its properties are all marked as unknown, and other places in IonBuilder should be freezing on some part of the objects which it traverses, such as through isOwnProperty().  In this case you only care about the prototype chain, but calling object->hasFlags(constraints, OBJECT_FLAG_UNKNOWN_PROPERTIES) in the loop below should work.

@@ +11188,5 @@
> +
> +        object = types::TypeObjectKey::get(proto);
> +    }
> +
> +    MOZ_CRASH("Unrechable");

typo

Comment 3

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea53aab8d7ce

Added a test for the mutating __proto__ thing that fails without the constraint (should have started with that I guess..).

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> This is apparently a pretty common case on Octane earley-boyer, patch wins
> at least a few hundred points there.

About 8% on AWFY. I checked and we're able to fold or optimize-to-IsObject all instanceof's on earley-boyer so it's pretty effective.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea53aab8d7ce

Comment 6

[Mayank Bansal]

Appears to have caused a 8% regression on Octane-navierstrokes on AWFY on machine 28 , http://arewefastyet.com/#machine=28&view=breakdown&suite=octane .

Comment 7

[Jan de Mooij [:jandem]]

(In reply to mayankleoboy1 from comment #6)
> Appears to have caused a 8% regression on Octane-navierstrokes on AWFY on
> machine 28 , http://arewefastyet.com/#machine=28&view=breakdown&suite=octane
> .

Yeah, pretty weird. Navier-stokes doesn't even use instanceof...

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Follow-up: add MIsObject::congruentTo

Minor follow-up patch to add congruentTo to MIsObject. Didn't want to file a separate bug just for this.

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8540127 [details] [diff] [review]
Follow-up: add MIsObject::congruentTo

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

Good find

Comment 10

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1747e8d827b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1747e8d827b

Comment 12

[Mayank Bansal]

Doesnt appear to have fixed the regression