602131 - Trace "in" for an array RHS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Arrays have a lookupProperty op, so trying to use "in" when the RHS is an array will trace and then OOM-exit (of all things).  We should just make this work (e.g. by hardcoding knowledge about the fact that the lookupProperty on Array is "safe" (which I bet it is) or having a way for class/object ops to indicate that or _something_.

Need this for sanely self-hosting array ops (or need another traceable way to detect holes).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Factor out the getting of capacity for dense arrays.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Factor out the getting of an array element's address for dense arrays.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make the 'in' operator trace usefully when its right-hand side is a dense array.

Comment 4

[Nicholas Nethercote [inactive]]

Re your third patch:  makeNumberInt32() has just changed its prototype (bug 601009).  You now have to check for whether it aborts.

Comment 5

[Nicholas Nethercote [inactive]]

Also, w.r.t. patches 1 and 2: the clean-ups are nice but will conflict with bug 602703, which will do a much bigger clean-up.  You can keep them there if you like but it'll probably be easier if you avoid much more stuff like that for the moment.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 481527 [details] [diff] [review]
part 1.  Factor out the getting of capacity for dense arrays.

I say njn is man enough to rebase across this.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 481528 [details] [diff] [review]
part 2.  Factor out the getting of an array element's address for dense arrays.

Looks good, just some comment nits:

> }
> 
> 
>         // Get the address of the element.
>-        LIns *dslots_ins =
>-            addName(lir->insLoad(LIR_ldp, obj_ins, offsetof(JSObject, dslots), ACCSET_OTHER), "dslots");
>-        JS_ASSERT(sizeof(Value) == 8); // The |3| in the following statement requires this.
>-        LIns *addr_ins = lir->ins2(LIR_addp, dslots_ins,
>-                                   lir->ins2ImmI(LIR_lshp, lir->insUI2P(idx_ins), 3));
>+        LIns *addr_ins = dense_array_element_address(obj_ins, idx_ins);

The comment doesn't seem necessary anymore. Maybe the one line of code here can slide into the next paragraph of code.

While you're in here, would you please delete the extra blank line before "// Get the address of the element."?

>     void set_array_fslot(nanojit::LIns *obj_ins, unsigned slot, uint32 val);
>+    /* array_obj_ins must be a dense array. */
>     nanojit::LIns* dense_array_capacity(nanojit::LIns *array_obj_ins);
>+    /**
>+     * array_obj_ins must be a dense array; idx_ins must be an index such that
>+     * 0 <= index < capacity.
>+     *
>+     * The return value is the address of the |index| element (the
>+     * address of the Value, in other words) in the array.
>+     */
>+    nanojit::LIns* dense_array_element_address(nanojit::LIns *array_obj_ins,
>+                                               nanojit::LIns *idx_ins);
> 

No /** in JS code. A simple /* will do. Also please add a blank line before each of these comments.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 481529 [details] [diff] [review]
part 3.  Make the 'in' operator trace usefully when its right-hand side is a dense array.

I think you have to:
        CHECK_STATUS(guardPrototypeHasNoIndexedProperties(obj, obj_ins, snapshot(MISMATCH_EXIT)));
r=me with that and some trace-tests.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> makeNumberInt32() has just changed its prototype

Thanks for the heads-up!  Updated to that; yay more copy/paste.  ;)

> it'll probably be easier if you avoid much more stuff like that 

Will do.  I just really didn't want to add a third copy of the complicated code...

> The comment doesn't seem necessary anymore.

Fixed.  I pulled the line of code up, not down, since we need addr_ins for more than just dealing with holes.

> No /** in JS code. A simple /* will do. Also please add a blank line before
> each of these comments.

Done.

> CHECK_STATUS(guardPrototypeHasNoIndexedProperties

Good catch!  Done.

Added some tests of basic functionality, and a test for the issue from comment 8.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Roll up patch for approval only (DO NOT CHECK IN)

Comment 11

[Boris Zbarsky [:bzbarsky]]

I had to do some nontrivial merging after bug 584917.  Jason, do you want to re-review the result?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 481527 [details] [diff] [review]
part 1.  Factor out the getting of capacity for dense arrays.

This is obsolete now that bug 602703 landed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 481528 [details] [diff] [review]
part 2.  Factor out the getting of an array element's address for dense arrays.

So is this.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to

Comment 15

[Boris Zbarsky [:bzbarsky]]

Sayrer, again, would be good to know whether I need to worry about this getting approval sometime....

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/b87a31d4c071

Comment 17

[Robert Sayre]

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