1122723 - IonMonkey: Fold MConcat with the empty string

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Super-simple patch. Shumway occasionally uses ""+x to induce ToString(x). This eliminates the concat operation.

Comment 1

[Sean Stangl [:sstangl]]

Attachment: patch

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 8550491 [details] [diff] [review]
patch

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

Good find.

Comment 3

[Till Schneidereit [:till]]

> Good find.

Indeed. It's also surprising we didn't encounter this in other hot code: it's one of the standard ways of coercing to string in JS, after all.

Comment 4

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65151ac03846

Comment 5

[Jeff Walden [:Waldo]]

> Shumway occasionally uses ""+x to induce ToString(x).

The two operations are not identical when |x| is an object without the default "valueOf" function property.  I skimmed the JITs a bit -- do we "handle" this by making MConcat use ConvertToStringPolicy, which boxes objects into values, then having it bail if the resulting value is ever an object at runtime?  That's desperately unclear from cursory examination of the code -- could we add comments "somewhere" to clarify string concatenation as being safe to use ToString, even tho according to spec it's not?

Comment 6

[Sean Stangl [:sstangl]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> I skimmed the JITs a bit -- do we "handle" this by making MConcat use
> ConvertToStringPolicy, which boxes objects into values, then having it bail
> if the resulting value is ever an object at runtime?

MConcat uses ConvertToStringPolicy for both arguments (MIR.h:6086), which wraps non-MIRType_String inputs with MToString (TypePolicy.cpp:398).

MToString uses a ToStringPolicy. ToStringPolicy::staticAdjustInputs() replaces incoming MIRType_Object inputs with MBox(MIRType_Object) (TypePolicy.cpp:687).
 
visitToString() then lowers MIRType_Value input to LValueToString. visitValueToString() then emits bail code for the MIRType_Object case (CodeGenerator.cpp:929).

Yeah comments would probably be nice.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65151ac03846