(I'm not sure where my comment on this went; anyway...) This renames the (dense) array allocation functions. They were named: js_NewEmptyArray js_NewPreallocatedArray NewArrayWithKind js_InitializerArray js_NewArrayObject NewDenseArrayObject which I think are pretty unintelligible. I'm not entirely satisfied with how the proto is given (ie, there are times we _know_ the proto is NULL, and we can't optimize that here), but it seems like the most elegant solution. I obviously don't like the names NewDenseAllocatedArrayDontSetTheLength and NewDenseUnallocatedArrayDontSetTheLength, but am at a loss for good names.

CC'ing people who might want to comment. Here are the new names: namespace js { JSObject *NewDenseEmptyArray(JSContext *cx, JSObject *proto=NULL); JSObject *NewDenseAllocatedArray(JSContext *cx, uint length, JSObject *proto=NULL); JSObject *NewDenseAllocatedArrayDontSetTheLength(JSContext *cx, uint length, JSObject *proto=NULL); JSObject *NewDenseUnallocatedArray(JSContext *cx, uint length, JSObject *proto=NULL); JSObject *NewDenseUnallocatedArrayDontSetTheLength(JSContext *cx, uint length, JSObject *proto=NULL); JSObject *NewDenseCopiedArray(JSContext *cx, uint length, Value *v); JSObject *NewSlowEmptyArray(JSContext *cx); } These look good to me. *DontSetTheLength's verbosity seems reasonable given its rarity and peculiarity. My one suggestion is to apply the "use _ as comma to separate clause from base name" style (as seen in other oddball functions like JS_IsConstructing_PossiblyWithGivenThisObject and ValueToString_TestForStringInline) giving NewDenseAllocatedArray_DontSetTheLength.

Updated to tracemonkey tip. In the meantime, some changes have allowed this patch become simpler. A side-effect of bug 606477 removed the need for the DontSetTheLength versions, which made it clear that the only parameter we needed was useCapacity.

Drive by bike shed: "dont" is a contraction without a contraction indicator -- DoNotSetLength has a letter where the apostrophe would otherwise be. (Will shut up now. :-)

It's OK, I've reduced it to this set of functions: namespace js { JSObject *NewDenseEmptyArray(JSContext *cx, JSObject *proto=NULL); JSObject *NewDenseAllocatedArray(JSContext *cx, uint length, JSObject *proto=NULL); JSObject *NewDenseUnallocatedArray(JSContext *cx, uint length, JSObject *proto=NULL); JSObject *NewDenseCopiedArray(JSContext *cx, uint length, Value *v); JSObject *NewSlowEmptyArray(JSContext *cx); }

Those all look good to me. (Glad to see "DontSetLength" gone, I too would have complained about it if it still were proposed. :-) )

(In reply to comment #7) > Those all look good to me. (Glad to see "DontSetLength" gone, I too would have > complained about it if it still were proposed. :-) ) Yeah, me too. There was a use case for it, but it was fortunately removed since the first patch.

I think you've attached the wrong patch :)

Whoops. Thanks for heads-up.

Comment on attachment 495599 [details] [diff] [review] Right patch this time Great patch, definitely more readable this way. Some nits: > * In dense mode, holes in the array are represented by (JS_ARRAY_HOLE) invalid >- * values. The final slot in fslots is unused. >+ * values. Since you are fixing this area, could you s/(JS_ARRAY_HOLE)/MagicValue(JS_ARRAY_HOLE)/. > * NB: the capacity and length of a dense array are entirely unrelated! The >+ * length may be greater than, less than, or equal to the capacity. The first >+ * case may occur when allocating an object before it is filled, as in >+ * js::NewDenseUnallocatedArray. See array_length_setter for another explanation >+ * of how the first case may occur. Instead of saying "before it is filled" which leads me to guess "'filled' by the implementation, as in, this is a temporarily inconsistent state?", which is totally wrong, perhaps you could be a bit more explicit and say, e.g.: The first case may occur when the user writes "new Array(100)", in which case the created array's length is 100 while the capacity is 0. (Thus, indices under the length but past the capacity must be treated as holes.) @@ -2334,17 +2335,16 @@ array_splice(JSContext *cx, uintN argc, /* If there are elements to remove, put them into the return value. */ if (count > 0) { if (obj->isDenseArray() && !js_PrototypeHasIndexedProperties(cx, obj) && - !js_PrototypeHasIndexedProperties(cx, obj2) && end <= obj->getDenseArrayCapacity()) { if (!InitArrayObject(cx, obj2, count, obj->getDenseArrayElements() + begin)) return JS_FALSE; } IIUC, you have removed every use of InitArrayObject except this one. Since, as you've demonstrated, InitArrayObject isn't really a coherent abstraction, rather, its just a simple factoring out of some code that was in 3 places, could you inline the body into this one use. That way the reader isn't asking "so... what does InitArrayObject do on an object that has already been obj->init()'d?" >- length = ValueIsLength(cx, vp + 2); >+ uintN length = ValueIsLength(cx, vp + 2); > if (vp[2].isNull()) > return JS_FALSE; >- vector = NULL; >+ obj = NewDenseUnallocatedArray(cx, length); While you are making arrays easier to understand, could you give ValueIsLength a sane name and interface. The canonical one that pops into my mind is: bool ValueToLength(JSContext *cx, const Value &v, jsuint *plength); with uses looking like every other ValueToX: jsuint length; if (!ValueToLength(cx, vp + 2, &length)) return JS_FALSE; >+namespace js { >+ >+namespace detail { >+template<bool useCapacity> >+static JS_ALWAYS_INLINE JSObject * >+NewArray(JSContext *cx, jsuint length, JSObject *proto, const Value* copy) No need for namespace 'detail' with static function in .cpp. >+ JS_ASSERT(proto); >+ JS_ASSERT(proto->isArray()); We generally don't assert non-null if the next statement will unequivocally crash on null. >+ >+ gc::FinalizeKind kind = GuessObjectGCKind(length, true); >+ JSObject* obj = js_NewGCObject(cx, kind); > if (!obj) > return NULL; > >- /* >- * If this fails, the global object was not initialized and its class does >- * not have JSCLASS_IS_GLOBAL. >- */ >- JS_ASSERT(obj->getProto()); >- >- return InitArrayObject(cx, obj, length, vector) ? obj : NULL; >+ /* JSObject::privateData represents length in Arrays */ >+ obj->init(cx, &js_ArrayClass, proto, proto->getParent(), >+ (void*)(length), true); >+ obj->setSharedNonNativeMap(); Assuming the compiler inlines appropriately, I don't see the advantage in open-coding this vs. just calling js::detail::NewObject. >+ if (useCapacity && !obj->ensureSlots(cx, length)) { >+ return NULL; >+ } No { } around single-statement then-branch. >+ if (copy) { >+ JS_ASSERT(useCapacity); >+ JS_ASSERT(obj->getDenseArrayCapacity() >= length); >+ memcpy(obj->getDenseArrayElements(), copy, length * sizeof(Value)); >+ } 4 space indent >+/* Create a dense array with length and capacity LENGTH. */ >+extern JSObject * JS_FASTCALL >+NewDenseAllocatedArray(JSContext *cx, uint length, JSObject *proto=NULL); Perhaps "with length and capacity == 'length'." ? >+/* >+ * Create a dense array with a set length, but without allocating space for the >+ * contents. Use this when accepting length from the user. >+ */ >+extern JSObject * JS_FASTCALL >+NewDenseUnallocatedArray(JSContext *cx, uint length, JSObject *proto=NULL); Perhaps "This is useful, e.g., when accepting length from the user."? >+/* Create a dense array with a copy of V. */ > extern JSObject * >-js_NewArrayObject(JSContext *cx, jsuint length, const js::Value *vector); >+NewDenseCopiedArray(JSContext *cx, uint length, Value *v, JSObject *proto=NULL); Canonical name is 'vp'. As above, instead of capitalizing in comment, perhaps name it with 'vp'. > if (withProto == WithProto::Class && !proto) { >- JSProtoKey protoKey = GetClassProtoKey(clasp); >- if (!js_GetClassPrototype(cx, parent, protoKey, &proto, clasp)) >- return NULL; >- if (!proto && !js_GetClassPrototype(cx, parent, JSProto_Object, &proto)) >- return NULL; >+ if (!FindProto (cx, clasp, parent, &proto)) >+ return NULL; > } With 99-char limit, can put "&& !FindProto(...) " on same line as "withProto == WithProto::Class && !proto". > RecordingStatus > TraceRecorder::newArray(JSObject* ctor, uint32 argc, Value* argv, Value* rval) > { ... >+ } else if (argc == 1 && argv[0].isNumber()) { >+ /* Abort on RangeError if the double doesn't fit in a uint. */ >+ LIns* num_ins; >+ CHECK_STATUS(makeNumberInt32(get(argv), &num_ins)); >+ >+ LIns *args[] = { proto_ins, num_ins, cx_ins }; >+ arr_ins = w.call(&js::NewDenseUnallocatedArray_ci, args); >+ guard(false, w.eqp0(arr_ins), OOM_EXIT); >+ >+ } else { You changed from using d2i to makeNumberInt32, and these don't seem equivalent in this case, particularly in the cases they optimize. If this was unintentional, you can just put it back to: LIns *args[] = { proto_ins, d2i(get(argv)), cx_ins }; If this was intentional, then it seems like it should go in a separate bug/patch so that its performance effects can be measured independently. r+ with those fixed as requested.

http://hg.mozilla.org/tracemonkey/rev/aae231781a45 (In reply to comment #11) > Comment on attachment 495599 [details] [diff] [review] > IIUC, you have removed every use of InitArrayObject except this one. Since, as > you've demonstrated, InitArrayObject isn't really a coherent abstraction, > rather, its just a simple factoring out of some code that was in 3 places, > could you inline the body into this one use. That way the reader isn't asking > "so... what does InitArrayObject do on an object that has already been > obj->init()'d?" I left this one out deliberately - I'm going to do it in bug 581180/bug 609896. > While you are making arrays easier to understand, could you give ValueIsLength > a sane name and interface. The canonical one that pops into my mind is: I committed with your r+, but please have a look and see if this is what you meant. > Assuming the compiler inlines appropriately, I don't see the advantage in > open-coding this vs. just calling js::detail::NewObject. One of the function's I replaced was open-coded, so I copied that. I've switched back to calling detail::NewObject. Swtiching back adds one branch, but it might be nicer to fix that more generally. > You changed from using d2i to makeNumberInt32, and these don't seem equivalent > in this case, particularly in the cases they optimize. If this was > unintentional, you can just put it back to: I think I groped around for a function with the right signature. I've switched to d2i.

(In reply to comment #12) > > You changed from using d2i to makeNumberInt32, and these don't seem equivalent > > in this case, particularly in the cases they optimize. If this was > > unintentional, you can just put it back to: > > I think I groped around for a function with the right signature. I've switched > to d2i. Switching to d2i was a mistake, and billm picked it up in bug 620029. This happened because d2i was sufficient in the first case, as lengths which were less than 0 were picked up by js_NewEmptyArray, but now aren't by js::NewDenseEmptyArray. Therefore it needs a guard, which makeNumberInt32 provides, but d2i doesn't.