551060 - GetArrayElement and SetArrayElement use a jsdouble index

Status: RESOLVED DUPLICATE of bug 924058

(Core :: JavaScript Engine, enhancement)

Description

[Andreas Gal :gal]

All call sites pass in jsuint. The fast path (dense array) inside *ArrayElement converts back to uint. Even the slow path shouldn't use double there (IndexToId). This will speed up a lot of Array.prototype.*() stuff.

Comment 1

[Jeff Walden [:Waldo]]

Be careful about making assumptions about speedups.  Most of that stuff already includes backdoors when it's known that setters won't come into play, and the backdoors avoid GetArrayElement and SetArrayElement both.  Still, there's likely more speedup to be had, just less than you'd think, I expect -- happy to see what lies in the darkness here.

Comment 2

[Andreas Gal :gal]

I am looking at splice for example.

jsuint length, begin, end, count, delta, last;
...
            for (last = begin; last < end; last++) {
                if (!JS_CHECK_OPERATION_LIMIT(cx) ||
	            !GetArrayElement(cx, obj, last, &hole, tvr.addr())) {
                    return JS_FALSE;
                }

We are talking a senseless int to double to int conversion here at every loop iteration. I am not saying slice() is frequently used and I am not saying this is critical to fix right away, but I think its worth filing & tracking a bug on it.

Comment 3

[Jeff Walden [:Waldo]]

Um, isn't that rather selective quoting?

2710         if (OBJ_IS_DENSE_ARRAY(cx, obj) && !js_PrototypeHasIndexedProperties(cx, obj) &&
2711             !js_PrototypeHasIndexedProperties(cx, obj2) &&
2712             end <= js_DenseArrayCapacity(obj)) {
2713             if (!InitArrayObject(cx, obj2, count, obj->dslots + begin,
2714                                  obj->fslots[JSSLOT_ARRAY_COUNT] !=
2715                                  obj->fslots[JSSLOT_ARRAY_LENGTH])) {
2716                 return JS_FALSE;
2717             }
2718         } else {
2719             for (last = begin; last < end; last++) {
2720                 if (!JS_CHECK_OPERATION_LIMIT(cx) ||
2721                     !GetArrayElement(cx, obj, last, &hole, tvr.addr())) {
2722                     return JS_FALSE;
2723                 }

The preceding block is precisely what guards against the cost you note applying in most cases.

But yes, please do file bugs -- just be careful about predicting wins, that's all.

Comment 4

[Andreas Gal :gal]

Not sure what we are arguing about? Are you saying int -> double -> int conversions are not expensive? And why selective quoting remark? You already pointed out that we try to bypass GetArrayElement/SetArrayElement for the dense array case, and I wasn't objecting. Why would I quote code that doesn't use the problematic path the bug is about?

I filed this bug as a note that we have a pointless uint -> double -> uint conversion. Fixing that will most definitely speed up paths using it. Some Array functions bypass it for a certain subset (probably large subset) of array objects. Other's do not (sort?). Even for the cases that bypass it for dense arrays, slow arrays are not some obscure thing we never operate on. They do happen. A lot actually.

I will drop the priority of the bug if that makes people happy, but I do think this deserves fixing when someone has a few free cycles.

Comment 5

[André Bargull [:anba]]

Fixed in bug 924058.