Closed Bug 476873 Opened 11 years ago Closed 11 years ago

TM: Trace JSOP_ARRAYPUSH

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

(Keywords: verified1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

No description provided.
Attached patch v1Splinter Review
Assignee: general → jorendorff
Attachment #360529 - Flags: review?(brendan)
Comment on attachment 360529 [details] [diff] [review]
v1

>+JS_FASTCALL JSBool
>+js_ArrayCompPush(JSContext *cx, JSObject *obj, jsval v)
>+{
>+    JS_ASSERT(OBJ_IS_DENSE_ARRAY(cx, obj));
>+    uint32_t length = (uint32_t) obj->fslots[JSSLOT_ARRAY_LENGTH];
>+    JS_ASSERT(length <= ARRAY_DENSE_LENGTH(obj));
>+    if (length == ARRAY_DENSE_LENGTH(obj) &&
>+        !ResizeSlots(cx, obj, ARRAY_DENSE_LENGTH(obj), length + 1)) {

Don't want one word realloc storms -- use ARRAY_GROWBY (from jsarray.h, see jsarray.cpp:EnsureLength).

r=me with that, thanks!

/be

P.S. Love seeing that Waldo banner after the added test!

>+test(testArrayComp2);
>+
> /*****************************************************************************
>  *                                                                           *
>  *  _____ _   _  _____ ______ _____ _______                                  *
>  * |_   _| \ | |/ ____|  ____|  __ \__   __|                                 *
>  *   | | |  \| | (___ | |__  | |__) | | |                                    *
>  *   | | | . ` |\___ \|  __| |  _  /  | |                                    *
>  *  _| |_| |\  |____) | |____| | \ \  | |                                    *
>  * |_____|_| \_|_____/|______|_|  \_\ |_|                                    *
Attachment #360529 - Flags: review?(brendan) → review+
Ridealong fodder, plus jorendorff more than earned the break hacking this.

/be
Flags: wanted1.9.1?
Duplicate of this bug: 467964
Ack!  I had it in my head that ResizeSlots uses ARRAY_GROWBY internally.

In fact, ARRAY_GROWBY (which is 8) seems too conservative to me.  Up to some generous limit we should double the array size each time we grow, right?  Might be a measurable win if any of the benchmarks make big arrays.  If you agree I'll file separately.
Flags: wanted1.9.1? → wanted1.9.1+
Violent agreement, will definitely help a bunch of cases.
Flags: wanted1.9.1+ → wanted1.9.1?
I know, I'll go back and reload so as not to clobber the change I midaired with.





OH WAIT!
Flags: wanted1.9.1? → wanted1.9.1+
(In reply to comment #5)
> Ack!  I had it in my head that ResizeSlots uses ARRAY_GROWBY internally.
> 
> In fact, ARRAY_GROWBY (which is 8) seems too conservative to me.  Up to some
> generous limit we should double the array size each time we grow, right?  Might
> be a measurable win if any of the benchmarks make big arrays.  If you agree
> I'll file separately.

Need at least ARRAY_GROWBY roundup here, but yeah: elsewhere in SpiderMonkey (we haven't consolidated into a lightweight C++ abstraction; maybe we should) we use binary-exponential growth up to some linear threshold, after which we grow by that size. Please file. I also suspect we want our dense array threshold to be more generous: any array of length <= 128 should be dense, e.g.

/be
http://hg.mozilla.org/tracemonkey/rev/527b21f9ab77
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/527b21f9ab77
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/e9226dd6c073
Flags: in-testsuite+
Flags: in-litmus-
js1_8_1/trace/trace-test.js
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.