462459 - TM: Better tracer support for |new Array(...)|

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jason Orendorff [:jorendorff]]

Currently we trace |new Array(7, 3, 9)| but not |new Array(7, 3)|.

We can continue to support |new Array()| and |new Array(number)| pretty much as we do now.  All other invocations of |new Array(...)| should be special-cased in TraceRecorder::functionCall.

Bug 462175 is orthogonal to this, but might be easy to fix at the same time.

Comment 1

[Jason Orendorff [:jorendorff]]

It turns out to be natural to address bug 462175 and this at the same time.

Unfortunately, my work in progress seems to reopen bug 455982. :-P

Also, there's a special case in here for:
  fn.apply(obj, [str])
which is a little hard to deal with.  I hope Andreas is rewriting that stuff in bug 462482; maybe that should block this.

Comment 2

[Brendan Eich [:brendan]]

Yes, bug 462482 will be patched to do away with all the SunSpider-specific apply hacks, which were temporary until we had time to do the right thing.

/be

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

I was confused--there's no need for me to touch the apply() code I was complaining about in comment 1.

This patch also fixes another bug: we were using the value of Array.prototype at record time.  ECMA requires us to use the "initial value of Array.prototype" per ECMA 262-3; the interpreter does the right thing and so does record_JSOP_NEWINIT.  Same fix for Date.prototype.

With this patch, I think it would be pretty easy to turn on JSOP_NEWARRAY.  Follow-up bug?

Comment 4

[Andreas Gal :gal]

I am about to obsolete the 1str case of apply. We should probably keep it until my patch has landed. My patch will not optimize for [str], but it will trace. We will be able to optimize for [arbitrary-array-init] and apply once Jason's patch has landed. We probably want a separate bug for that as a follow up once both patches are in.

Comment 5

[Andreas Gal :gal]

Comment on attachment 345761 [details] [diff] [review]
v1

- Could we rename 1int to something more meaningful? Like FastNewArrayWithLength?

- The argc == 1 should be simplified once 1str is no longer needed. It looks really confusing.

Comment 6

[Andreas Gal :gal]

Do we have test coverage for the various array cases?

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to comment #3)
> This patch also fixes another bug: we were using the value of Array.prototype
> at record time.  ECMA requires us to use the "initial value of Array.prototype"
> per ECMA 262-3; the interpreter does the right thing and so does
> record_JSOP_NEWINIT.  Same fix for Date.prototype.

Correction: This isn't a bug because Array.prototype and Date.prototype are READONLY and PERMANENT.  v2 reverts the implementation (but still factors this out into a method).

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v2

Carrying forward Andreas's r+.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #7)
> (In reply to comment #3)
> > This patch also fixes another bug: we were using the value of Array.prototype
> > at record time.  ECMA requires us to use the "initial value of Array.prototype"
> > per ECMA 262-3; the interpreter does the right thing and so does
> > record_JSOP_NEWINIT.  Same fix for Date.prototype.
> 
> Correction: This isn't a bug because Array.prototype and Date.prototype are
> READONLY and PERMANENT.  v2 reverts the implementation (but still factors this
> out into a method).

Catching up on bugmail -- yes, this works because of the immutable .prototype binding in standard constructors, and because we shape-guard (or else guard on the identity of a "shapeless" callee) on the constructor, which means it is sure to remain Array (the real one ;-) on trace.

Patch looks good!

/be

Comment 10

[Jason Orendorff [:jorendorff]]

Raytrace is crashing this patch. :-P

Comment 11

[Jason Orendorff [:jorendorff]]

fix:

         // arr->dslots[i] = box_jsval(vp[i]);  for i in 0..argc
-        LIns *dslots_ins;
+        LIns *dslots_ins = NULL;
         for (uint32 i = 0; i < argc; i++) {
             LIns *elt_ins = get(argv + i);
             if (!box_jsval(argv[i], elt_ins))
                 return false;
             stobj_set_dslot(arr_ins, i, dslots_ins, elt_ins, "set_array_elt");
         }

Comment 12

[Jason Orendorff [:jorendorff]]

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

Comment 13

[Bob Clary [:bc] (inactive)]

Array called as a function isn't traced. New bug?

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> Array called as a function isn't traced. New bug?

Yes, please.

/be

Comment 15

[Bob Clary [:bc] (inactive)]

Attachment: js1_8_1/trace/regress-462459-01.js, ...

tests for Array(...), new Array(...) and []. I used separate tests since nested functions and evald code killed the trace. :-(

js1_8_1/trace/regress-462459-{01,02,03,04}.js fail.

Comment 16

[Bob Clary [:bc] (inactive)]

Bug 476207

Comment 17

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/238c9ca169fd

Comment 18

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2 modulo Bug 476207 js1_8_1/trace/regress-462459-{01,02,03,04}.js