Closed Bug 1113677 Opened 5 years ago Closed 5 years ago

Fold instanceof based on type information

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
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.
Attachment #8539293 - Flags: review?(bhackett1024)
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.
Flags: needinfo?(bhackett1024)
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
Attachment #8539293 - Flags: review?(bhackett1024) → review+
Flags: needinfo?(bhackett1024)
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..).
(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.
https://hg.mozilla.org/mozilla-central/rev/ea53aab8d7ce
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Appears to have caused a 8% regression on Octane-navierstrokes on AWFY on machine 28 , http://arewefastyet.com/#machine=28&view=breakdown&suite=octane .
(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...
Minor follow-up patch to add congruentTo to MIsObject. Didn't want to file a separate bug just for this.
Attachment #8540127 - Flags: review?(hv1989)
Comment on attachment 8540127 [details] [diff] [review]
Follow-up: add MIsObject::congruentTo

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

Good find
Attachment #8540127 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1747e8d827b
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/c1747e8d827b
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Doesnt appear to have fixed the regression
Blocks: 1115531
No longer blocks: 1115531
Depends on: 1115531
You need to log in before you can comment on or make changes to this bug.