582081 - dense array patches regressed empty Array creation on Dromaeo

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Sayre]

see URL field for Dromaeo comparison. Andreas says this is because he intentionally changed arrays to allocated their length immediately

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Andreas Gal :gal]

This patch moves dense array capacity into an fslot and allows dslots to be NULL again. This is sadly needed for new Array(large-number) which then is never used.

Comment 5

[Jeff Walden [:Waldo]]

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

>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp

>-    /* dslots can be briefly NULL during array creation */
>+    /* dslots can be NULL during array creation */

Add a period at the end of this complete sentence while you're here.


>@@ -3046,21 +3050,18 @@ js_NewArrayObject(JSContext *cx, jsuint 
>         return NULL;
> 
>     /*
>      * If this fails, the global object was not initialized and its class does
>      * not have JSCLASS_IS_GLOBAL.
>      */
>     JS_ASSERT(obj->getProto());
> 
>-    {
>-        AutoObjectRooter tvr(cx, obj);
>-        if (!InitArrayObject(cx, obj, length, vector))
>-            obj = NULL;
>-    }
>+    if (!InitArrayObject(cx, obj, length, vector))
>+        obj = NULL;

InitArrayObject may not do anything involving the GC, but relying on this (and requiring the reader to understand this) seems quite fragile to me.  As long as we're not relying on conservative stack scanning, I would prefer leaving this rooter in place.


>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

>         /*
>          * Remember the class this function is a constructor for so that
>          * we know to create an object of this class when we call the
>-         * constructor.
>+         * constructor. Arrays are a special case. We have a slow and a
>+         * dense array. While the prototype is a slow array (it has
>+         * named properties), we want to make dense arrays with the
>+         * constructor, so we have to monkey patch that here.
>          */
>-        FUN_CLASP(fun) = clasp;
>+        FUN_CLASP(fun) = (clasp == &js_SlowArrayClass) ? &js_ArrayClass : clasp;

Ugly.  I wonder if we might not be cleaner not relying on the standard class-initialization mechanism.  But that's not particularly important, so I guess this works reasonably enough.


>diff --git a/js/src/jsobj.h b/js/src/jsobj.h

>+    // Used by dense arrays only.
>+    static const uint32 JSSLOT_ARRAY_CAPACITY = JSSLOT_PRIVATE + 1;

JSSLOT_DENSE_ARRAY_CAPACITY?  Self-describing that way.


>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp

>-    if (argc == 0) {
>-        // arr_ins = js_NewEmptyArray(cx, Array.prototype)
>-        LIns *args[] = { proto_ins, cx_ins };
>+    if (argc == 0 || (argc == 1 && argv[0].isNumber())) {
>+        LIns *args[] = { argc == 0 ? lir->insImmI(0) : d2i(get(argv)), proto_ins, cx_ins };
>         arr_ins = lir->insCall(&js_NewEmptyArray_ci, args);
>         guard(false, lir->insEqP_0(arr_ins), OOM_EXIT);
>-    } else if (argc == 1 && argv[0].isNumber()) {
>-        // arr_ins = js_NewEmptyArray(cx, Array.prototype, length)
>-        LIns *args[] = { d2i(get(argv)), proto_ins, cx_ins }; // FIXME: is this 64-bit safe?
>-        arr_ins = lir->insCall(&js_NewArrayWithSlots_ci, args);
>-        guard(false, lir->insEqP_0(arr_ins), OOM_EXIT);
>     } else {
>-        // arr_ins = js_NewArrayWithSlots(cx, Array.prototype, argc)
>         LIns *args[] = { INS_CONST(argc), proto_ins, cx_ins };
>-        arr_ins = lir->insCall(&js_NewArrayWithSlots_ci, args);
>+        arr_ins = lir->insCall(&js_NewPreallocatedArray_ci, args);
>         guard(false, lir->insEqP_0(arr_ins), OOM_EXIT);
> 
>         // arr->dslots[i] = box_jsval(vp[i]);  for i in 0..argc
>         LIns *dslots_ins = NULL;
>         for (uint32 i = 0; i < argc && !outOfMemory(); i++) {
>             stobj_set_dslot(arr_ins, i, dslots_ins, argv[i], get(&argv[i]));
>         }
>     }

This points out a pre-existing problem: on trace we currently don't throw when encountering numeric, non-unsigned-32-bit integer lengths -- 3.5, -1, NaN, infinities, &c.  (You'll fix the negative, signed 32-bit case, but fractions and nonfinites will remain incorrect.)  I filed bug 582161 to fix this.


The JSOP_NEWINIT changes point out to me that JSOP_INITELEM generates code that checks array length/capacity before assigning.  In some cases (sharps, generators, I think that might be it) this seems necessary.  But for more normal arrays, it may not be.  Does web code generate (and trace) newinit'd arrays enough for it to matter?  Maybe, maybe not: more data could be helpful.

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/85ac7407fdf9

sayrer asked me to land this, so I made the requested tweaks and did so.

Comment 7

[Andreas Gal :gal]

Always ok to steal from me. Thanks for landing this for me.

Comment 8

[Igor Bukanov]

(In reply to comment #6)
> http://hg.mozilla.org/tracemonkey/rev/85ac7407fdf9
> 
> sayrer asked me to land this, so I made the requested tweaks and did so.

This broke login into GMail for domains. I.e. after entering the passoword GMail shows a login progress page and then just hangs when the progress bar show near full load. This with jit on or off.

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> This broke login into GMail for domains. I.e. after entering the passoword
> GMail shows a login progress page and then just hangs when the progress bar
> show near full load. This with jit on or off.

Also there is a message on sterr:

*** e = [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: chrome://browser/content/utilityOverlay.js :: getShellService :: line 307"  data: no]

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Also there is a message on sterr:
> 
> *** e = [Exception... "Component returned failure code: 0x80570016
> (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult:
> "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame ::
> chrome://browser/content/utilityOverlay.js :: getShellService :: line 307" 
> data: no]

This also present without the patch, so this is unrelated. Note also that the bug presnt both in debug and optimized builds and does not show up in any other way AFAICS.

Comment 11

[Igor Bukanov]

Should this be backed out?

Comment 12

[Robert Sayre]

Let's try to fix it. Any chance of a reduced test case?

Comment 13

[Igor Bukanov]

The source for the page that shows the progress bar shows that it contains all the information to show on the page after loading, but that information comes in the form of array literals. Those array literals contain a lot holes and empty arrays. It could be that the changes broke somehow the literal initialization in this case? 

If this does not trigger any bells, I plan to add to the interpreter dumping of all constructed literals and then try to diff the dump before and after the patch.

Comment 14

[Robert Sayre]

Andreas and I can reproduce with regular GMail, and we get an error about invalid array length. Looking in debug builds.

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> Andreas and I can reproduce with regular GMail, and we get an error about
> invalid array length. Looking in debug builds.

I was so certain that somebody has already tested this under ordinary gmail so I have not checked that self. But two days and halve days are were not enough for this to propagate...

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/85ac7407fdf9