828119 - IonMonkey: Handle polymorphic stricteq better

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: microbenchmark

earley-boyer's sc_assq function is called with |o| being number|string, and this feeds into a === where the other side is always a string. We generate a VM call for this. 

For this case we should be able to generate something much better. On the attached benchmark, we get 313ms, and 116ms when the types are monomorphic. v8 gets 85ms even when the types are polymorphic.

This seems to be about 10% of the runtime of boyer, and boyer is about 80% of the runtime of earley-boyer. I'm estimating this to be about a 4-5% speedup based on profiles, but it's hard to tell. sc_assq overall is a large part of boyer's runtime.

Comment 1

[Tom S [:evilpie]]

V8 uses an IC for this. This means after the code is compiled in crankshaft, there is only string comparison being observed.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Cool, that could work. A quick hack would be to compile it to some kind of LCompareS lookalike that does a type check, and:
 (1) string compares if both are string
 (2) bails if neither are string
 (3) branches to false if one is not a string

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Add LCompareStrictS

As you did my previous MCompare review, I'm assigning you for the review. If you want to nominate somebody else, go ahead.

This adds LCompareStrictS that does the same as LCompareS, but now with lhs a Value and rhs a String. If lhs is a string it does the LCompareS logic, else it will set the output immediatly. String === Something is always false if Something isn't a String.

My own microbenchmark goes from 800ms to 250ms. And seeing a 6% improvement on looping the boyer part of earley-boyer for 100 times. I assume this will correspond to the 4%-5% Dave measured on the whole benchmark.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 708533 [details] [diff] [review]
Add LCompareStrictS

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

Nice! r=me with comments below addressed.

::: js/src/ion/CodeGenerator.cpp
@@ +2428,5 @@
> +
> +bool
> +CodeGenerator::visitCompareStrictS(LCompareStrictS *lir)
> +{
> +    JSOp op = lir->mir()->jsop();

Nit: JS_ASSERT(op == JSOP_STRICTEQ || op == JSOP_STRICTNE);

@@ +2447,5 @@
> +        return false;
> +
> +#if defined(JS_PUNBOX64)
> +    // Restoring string tag is only needed when tag and payload where in 1 register.
> +    masm.tagValue(JSVAL_TYPE_STRING, left.scratchReg(), left);

This is a bit unfortunate, and I'm not sure if clobbering |left| works well with the OOL VM call in emitCompareS (the safepoint has a list of live registers etc). It would be simpler to use a second temp instead of left.scratchReg() (I don't think it matters for performance - the important part is that we avoid a VM csll)

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +596,5 @@
>      Register extractObject(const Address &address, Register scratch);
>      Register extractObject(const ValueOperand &value, Register scratch) {
>          return value.payloadReg();
>      }
> +    Register extractString(const Address &address, Register scratch) {

Nit: move these to IonMacroAssembler.h

Comment 5

[Hannes Verschore [:h4writer]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Hannes Verschore [:h4writer]]

2.8% / 3.6% on earley-boyer v8 / octane