672076 - TM: incelem index may be converted to id multiple times

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Bug 647624 added this test for the number of times the index is converted to a string id in incelem/etc. ops:


var counter = 0;
var x = { toString: function() { counter++; } };
var y = {};

for (var i = 0; i < 50; i++)
  ++y[x];

assertEq(counter, 50);


> js -j test.js
Error: Assertion failed: got 52, expected 50


BTW, Firefox seems to be the only browser that implements this behavior for incelem (other engines will convert 'x' to an id twice), but according to Waldo we're doing the right thing here.

Comment 1

[Jeff Walden [:Waldo]]

The problem is that we fall off trace in the middle of the incelem opcode, roughly.  That falls into an imacro, and the imacro doesn't properly implement the element-increment special form.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: wip v1

Comment 3

[Shu-yu Guo [:shu]]

(In reply to comment #2)
> Created attachment 548970 [details] [diff] [review] [review]
> wip v1

I would suggest just tracing the decomposed versions of INCELEM/DECELEM as done by bhackett in http://hg.mozilla.org/projects/jaegermonkey/rev/73578

In particular, JSOP_TOID would be much preferable to that special bail op.

Comment 4

[Brian Hackett [Laid off!]]

Yeah, talked to dvander on IRC yesterday and I'm going to do another followup (should be later today) to remove the implementation of element incops entirely (including the imacros) and have the tracer run on the decomposed version.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch removing incelem imacros

Patch removing incelem code from the tracer, interpreter and imacros.  I was not able to repro the flat change from bug 647624 comment 4, and get a regression on a dense array eleminc microbenchmark for the tracer from 42ms -> 75ms.  I looked at the before/after disassembly and on the decomposed version there are an seven extra inline stack stores and an extra hole check at the setelem.  The reason for this is that the tracer does not always invoke the imacro, but has a separate path used for elem incops on dense arrays.

So this patch is here for reference and I don't think it should go in.  Tom, it should not be hard to update your patch to use the TOID opcode in place on the JM branch, which will avoid needing a second opcode for this language corner case.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to comment #5)
> Created attachment 549177 [details]
> patch removing incelem imacros
> 
> Patch removing incelem code from the tracer, interpreter and imacros.  I was
> not able to repro the flat change from bug 647624 comment 4, and get a
> regression on a dense array eleminc microbenchmark for the tracer from 42ms
> -> 75ms.

Where can I get this benchmark?

Comment 7

[Brian Hackett [Laid off!]]

Here is the full one I tested:

var start = new Date;

function foo(x, y) {
  for (var i = 0; i < 1000000; i++)
    ++x[y];
}
for (var i = 0; i < 10; i++) {
  var a = new Array();
  a[0] = 0;
  foo(a, 0);
}

print(new Date - start);

I get these times:

js -j (old): 42
js -j (with patch): 75
js -m (with bug 647624 patched): 96
js -m -n: 30
d8: 36

Comment 8

[Shu-yu Guo [:shu]]

(In reply to comment #7)
> Here is the full one I tested:
> 
> var start = new Date;
> 
> function foo(x, y) {
>   for (var i = 0; i < 1000000; i++)
>     ++x[y];
> }
> for (var i = 0; i < 10; i++) {
>   var a = new Array();
>   a[0] = 0;
>   foo(a, 0);
> }
> 
> print(new Date - start);
> 
> I get these times:
> 
> js -j (old): 42
> js -j (with patch): 75
> js -m (with bug 647624 patched): 96
> js -m -n: 30
> d8: 36

I have a patch that makes the tracer generate the same LIR for both the decomposed version and the fat version. The decomposed version is still slower on trace, which I guess is because of the extra stack writes.

js -j: 43
js -n -a -j: 53

Comment 9

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

What if we just don't trace weird keys as indexes? INCELEM is already pretty rare, I would take the perf regression from removing the imacros entirely.

Comment 10

[Shu-yu Guo [:shu]]

Attachment: Packed dense arrays in tracer

In conjunction with bhackett's patch, this patch results in the same LIR being generated (modulo extra stack writes) for dense arrays as the old fastpath.

Comment 11

[Shu-yu Guo [:shu]]

For anyone's curiosity: the reason that the intermediate stack writes cannot be optimized away is that pre-decomposition all guards bailed back to the start of the INCELEM/DECELEM op, whereas now guards are peppered throughout the decomposed op, which if taken, require the stack be reflected back to the interpreter.

Comment 12

[Tom S. (please needinfo tschuster)]

Tracer has been removed.