545165 - Bug in order of evaluation with JSOP_CONCATN

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

function g() {
    var log = '';
    var a = {toString: function () { log += 'a'; return 'A'; }};
    function b(x) { log += 'b'; return 'B'; }
    assertEq("[" + a + b() + "]", "[AB]");
    assertEq(log, "ab");
}
g();

Comment 1

[Luke Wagner [:luke]]

Attachment: basic fix

The problem is that the parser (1) emits code to evaluate each of the operands, then (2) emits JSOP_CONCATN.  (1) evaluates the code down to a value, not a string, so that (2) must perform string conversion.  This produces the sequence:

  eval(arg1), ..., eval(argN), arg1.toString(), ... argN.toString()

Which should be:

  eval(arg1), arg1.toString(), ..., eval(argN), argN.toString()

The obvious fix is to emit extra ops, after each operand, to take the value to a string.  However, this also gives away some of JSOP_CONCATN's potential speedup since JSOP_CONCATN avoids creating JSString objects for non-string primitive arguments.  OTOH, I suppose string concatenation of non-string primitives is the rare case.

Now, for the exact op to emit after each operand, what is needed is something that implements E-262-3 section 9.8's toString, as implemented by js_ValueToString/js_ValueToCharBuffer.  Grepping jsops.cpp for an op that just calls js_ValueToString on sp[-1], I see JSOP_XMLTAGEXPR.  This isn't XML, but... Yoink!  The attached patch fixes jorendorff's test case above (and the one from IRC).

Still needed:
 - perhaps this confuses the disassembler
 - this allows JSOP_CONCATN to be simpler and the recording thereof to be MASSIVELY simpler
 - avoid emitting JSOP_XMLTAGEXPR for string literals.  (Are there other cases where it is trivial to tell that an expression tree is already a string?)

Comment 2

[Luke Wagner [:luke]]

(In reply to comment #1)
> Still needed:

Oh yeah and:
 - rename JSOP_XMLTAGEXPR to JSOP_STRINGIFY so I don't kill Waldo.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 426029 [details] [diff] [review]
basic fix

Yeah, this will break the decompiler. Suggest adding JSOP_STRINGIFY and avoiding the XML ghetto, soon to be relocated to desugar-to-builtin-calls by igor.

/be

Comment 4

[Luke Wagner [:luke]]

Attachment: fix

I am very happy about this bug; the resulting code is way simpler.  I wish I hadn't been locked into thinking "cannot change JSOP_CONCATN -- must only copy its semantics".  Also taking out imacro tail-call since it was added just for JSOP_CONCATN.

Also, I realized that a priori string conversion is only necessary for non-primitives.  This cuts down on the number of temporary strings created.

Comment 5

[Luke Wagner [:luke]]

Also, I'm not that familiar with the disassembler, but it seems that the default behavior (don't touch the stack) generates the right output AFAICS since JSOP_OBJTOSTRING is invisible.  Can anyone figure differently?  Cc'ing people who might...

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 427900 [details] [diff] [review]
fix

Drive-by: something about JSOP_OBJTOSTRING seems off -- maybe OBJ abbreviation vs. STRING being spelled out. OBJECTTOSTRING is too long and has TT run together.

OBJTOSTR? I liked STRINGIFY. JSOP_TOSTRING comes to mind and implies "object" to left of "to" for anyone who knows JS (the language; not the Ecma specs).

/be

Comment 7

[Luke Wagner [:luke]]

I didn't do JSOP_STRINGIFY since the op only stringifies if the operand is an object, otherwise leaves it a primitive. Since |(1).toString()| evaluates to "1", JSOP_TOSTRING also seems a misnomer.  I do agree with the point about irregular abbreviation; I am happy to change to JSOP_OBJTOSTR.

Comment 8

[Brendan Eich [:brendan]]

Is it better to have js_ValueToCharBuffer do the non-string-primitive-to-string conversion inside JSOP_CONCATN, if whenever there's a variable referring to the non-string-primitive (!pn2->isLiteral()) you have to dispatch a JSOP_OBJTOSTR, which does nothing in the non-string-primitive case?

The values of better include clearer division of labor and greater symmetry, as well as raw interpreter performance. It seems the JIT can cope either way, but again saying it with bytecode instead of native code usually means smaller code footprint (less duplication -- this was the impetus for Tamarin Tracing's Forth).

/be

Comment 9

[Luke Wagner [:luke]]

(In reply to comment #8)
> Is it better to have js_ValueToCharBuffer do the non-string-primitive-to-string
> conversion inside JSOP_CONCATN, if whenever there's a variable referring to the
> non-string-primitive (!pn2->isLiteral()) you have to dispatch a JSOP_OBJTOSTR,
> which does nothing in the non-string-primitive case?

Performance-wise, it is significantly better to delay as much conversion as possible until the CONCATN.  I do agree that the division is fuzzy since "some" conversion is happening before CONCATN and "some" is happening during.  The division is precise, though: conversion to primitive before CONCATN, conversion from primitive during.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Performance-wise, it is significantly better to delay as much conversion as
> possible until the CONCATN.

That's true of the interpreter, no doubt. For the tracer, does it matter?

/be

Comment 11

[Luke Wagner [:luke]]

(In reply to comment #10)
> That's true of the interpreter, no doubt. For the tracer, does it matter?

The tracer actually uses a worse algorithm: it converts all arguments to strings, puts these into a lir->alloc'd array, and passes the array to a builtin to do concatenation.  (I also tried the other way (building a heterogeneous array of type-tagged data on trace and passing this to the builtin), but this was slower for most cases.)

Comment 12

[Luke Wagner [:luke]]

Also, since JM emits per-bytecode, JM would benefit from delayed stringification.

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 427900 [details] [diff] [review]
fix

*steal*

Looks great.

Comment 14

[Jason Orendorff [:jorendorff]]

I forgot one nit — please do rename JSOP_OBJTOSTRING to JSOP_OBJTOSTR.

Thanks!

Comment 15

[Brendan Eich [:brendan]]

Yeah, this patch is good, sorry for bothering about the symmetry breaking. The symmetry breaking is for winning performance reasons, and the code compared to what was there before is better all around. The only thing to do is more static type inference so we can avoid some JSOP_OBJTOSTR emits, but that is for later bugs.

/be

Comment 16

[Luke Wagner [:luke]]

Thanks!

http://hg.mozilla.org/tracemonkey/rev/e5b7c34d5a31

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e5b7c34d5a31