The typed array class hierarchy is a bit of a mess. It should be cleaned up, and JSObject::isTypedArray() will be a casualty.

I hope adding |DeclMarker(Object, ArrayBufferViewObject)| is appropriate... it seems to work. There's one really dodgy looking bit of pre-existing code: the DEBUG-only block in ArrayBufferObject::obj_trace. I had to reinterpret_cast an ArrayBufferObject to an ArrayBufferViewObject, which indicates that the pre-existing code might have bad types, though I don't really understand how the links between the ArrayBufferObjects work. Interestingly, that block is never executed during jit-tests or jstests, so I suspect it's unreachable. Clarity on that block would be welcome!

This patch just moves some methods from jstypedarrayinlines.h to jstypedarray.h, and removes some redundant assertions.

Comment on attachment 770036 [details] [diff] [review] (part 2) - Rename ArrayBuffer as ArrayBufferObject in comments, where appropriate. Review of attachment 770036 [details] [diff] [review]: ----------------------------------------------------------------- I don't think all of these s/ArrayBuffer/ArrayBufferObject/ are valid. In many cases, I was referring to the ArrayBuffer in the spec, not the ArrayBufferObject class that implements it. To avoid churn, perhaps it'd be easiest to apply this patch, then I can add a patch on top to revert the parts of this I disagree with.

Comment on attachment 770038 [details] [diff] [review] (part 4) - Rename DataView as DataViewObject in comments, where appropriate. Review of attachment 770038 [details] [diff] [review]: ----------------------------------------------------------------- I think all these are fine, since they all really refer to the implementation versions.

> I don't think all of these s/ArrayBuffer/ArrayBufferObject/ are valid. In > many cases, I was referring to the ArrayBuffer in the spec, not the > ArrayBufferObject class that implements it. I tried to account for that -- I left a bunch of them unchanged. In a lot of cases it's not clear, esp. when there's a 1-to-1 correspondence between the two. > To avoid churn, perhaps it'd be easiest to apply this patch, then I can add > a patch on top to revert the parts of this I disagree with. I'm not worried about churn, so feel free to just tell me directly the parts you disagree with.

Comment on attachment 770047 [details] [diff] [review] (part 8) - Move some functions into ArrayBufferViewObject and make its SLOT members protected. Review of attachment 770047 [details] [diff] [review]: ----------------------------------------------------------------- This makes things way nicer! ::: js/src/jstypedarray.cpp @@ +688,5 @@ > bool found = false; > + int x = 0; > + for (ArrayBufferObject *p = obj->compartment()->gcLiveArrayBuffers; > + p; > + p = reinterpret_cast<ArrayBufferViewObject*>(p)->bufferLink()) This looks wrong. I think it should be p = (*GetViewList(p))->bufferLink(); which does mean that this is never getting run by the test suite. :( It's actually really hard to get this to trigger. You have to have obj_trace() called multiple times on the same object, which only happens when you overflow the mark stack. The shell seems to leave the mark stack size limit set to 2^64-1, which is not helpful. I have a patch that exposes the stack limit tuning parameter to gcparam(), and it manages to invoke this test code. And immediately crashes because the current code is completely bogus. If I switch to the current equivalent of the code I wrote above (it's a lot uglier before your changes), it passes the test. @@ +776,5 @@ > { > for (ArrayBufferObject **p = vector.begin(); p != vector.end(); p++) { > ArrayBufferObject *buffer = *p; > JSCompartment *comp = buffer->compartment(); > + ArrayBufferViewObject *firstView = *GetViewList(&buffer->as<ArrayBufferObject>()); What's this ->as<> for? ::: js/src/jstypedarray.h @@ +239,5 @@ > + void setNextView(ArrayBufferViewObject *view) { > + setFixedSlot(NEXT_VIEW_SLOT, PrivateValue(view)); > + } > + > + void prependViews(HeapPtr<ArrayBufferViewObject> *views); How about prependToViews?

Comment on attachment 770051 [details] [diff] [review] (part 10) - Clean up ArrayBufferObject a bit. Review of attachment 770051 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the very nicely split up patch series! ::: js/src/jstypedarray.h @@ +168,5 @@ > + header->length = 0; > + header->capacity = 0; > + } > + > + static uint32_t getElementsHeaderInitializedLength(const js::ObjectElements *header) { Is this needed at all anymore? Seems like it can be a direct access now.

Comment on attachment 770036 [details] [diff] [review] (part 2) - Rename ArrayBuffer as ArrayBufferObject in comments, where appropriate. Review of attachment 770036 [details] [diff] [review]: ----------------------------------------------------------------- You know, I take it back. I'm fine with this patch as-is. I was thinking of ArrayBuffers having views at the abstract level, as opposed to ArrayBufferObjects having a linked list of ArrayBufferViewObjects, but really both are equally valid and in almost all of the specific comments, the latter makes more sense.

> @@ +776,5 @@ > > { > > for (ArrayBufferObject **p = vector.begin(); p != vector.end(); p++) { > > ArrayBufferObject *buffer = *p; > > JSCompartment *comp = buffer->compartment(); > > + ArrayBufferViewObject *firstView = *GetViewList(&buffer->as<ArrayBufferObject>()); > > What's this ->as<> for? I want to be *really* sure it's an ArrayBufferObject? Good catch, and I found another case just like it.

This patch is mostly tedium and monotony. But there's one interesting change, relating to TypedArrayObject::neuter(). One of the slots it clears is LENGTH_SLOT, which is a TypedArrayObject-specific slot. However, the function is called on objects of type ArrayBufferViewObject, which includes DataViewObjects which lack the LENGTH_SLOT(!) So I added DataViewObject::neuter() and ArrayBufferViewObject()::neuter(), which dispatches to either of DataViewObject::neuter() or TypedArrayObject::neuter(). BTW, JS_GetObjectAsArrayBufferView() checks that its arg is an ArrayBufferView and returns NULL if so, but all the other typed array friend API functions just assert that the arg is an ArrayBufferView. Doesn't seem ideal.

(Here's the comment that I forgot to attach to part 13 when I uploaded.) This patch changes byteLengthValue(tarr).toInt32() to the equivalent tarr->byteLength() in several places. And likewise for byteOffset(), and length().

DataViewObject::byteLength() reads a Value from a slot, then converts it to a uint32_t. DataViewObject::byteLengthValue() calls byteLength(), then converts the resulting uint32_t *back* to a Value. Same story with byteOffset() and byteOffsetValue(). This patch inverts these functions so they are more like the equivalents in TypedArrayObject, thus avoiding the unnecessary Value -> uint32_t -> Value round-trip in the *Value() functions.

This patch moves the few remaining things in jstypedarrayinlines.h into jstypedarray.{cpp,h} and deletes it. Yay! Notice that two of the files that #included jstypedarrayinlines.h were vanilla .h files, tut tut. I had to add some new #include statements to account for headers that things that were being pulled in indirectly, via jstypedarrayinlines.h

This is the last patch in the series. I chose |TypedArrayObject| for the filename because it's the most representative/generic of the objects in the file. (I did something similar with vm/ProxyObject.{h,cpp} in bug 887558.)

Parts 1--10: https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed886b5122b https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd37861c48c https://hg.mozilla.org/integration/mozilla-inbound/rev/13dd71c1b822 https://hg.mozilla.org/integration/mozilla-inbound/rev/64cdd849567d https://hg.mozilla.org/integration/mozilla-inbound/rev/909027ddcb43 https://hg.mozilla.org/integration/mozilla-inbound/rev/fe791e969c9d https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ac4720b52b https://hg.mozilla.org/integration/mozilla-inbound/rev/3a943844f56f https://hg.mozilla.org/integration/mozilla-inbound/rev/18dfc741376f https://hg.mozilla.org/integration/mozilla-inbound/rev/e4ee38bb3715

Comment on attachment 770571 [details] [diff] [review] (part 11) - Move some functions into TypedArrayObject and make its SLOT members private. Review of attachment 770571 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch on the neuter(). I was worried that it might be s-s, but it looks like it was just zeroing out an unused slot before, since it was using the 8-slot allocKind.

Comment on attachment 770598 [details] [diff] [review] (part 13) - Use appropriate methods from TypedArrayObject where appropriate. Review of attachment 770598 [details] [diff] [review]: ----------------------------------------------------------------- Was there a part 12?

Comment on attachment 770617 [details] [diff] [review] (part 15) - Remove jstypedarrayinlines.h. Review of attachment 770617 [details] [diff] [review]: ----------------------------------------------------------------- \o/

Comment on attachment 770621 [details] [diff] [review] (part 16) - Rename jstypedarray.{h,cpp} as vm/TypedArrayObject.{h,cpp}. Review of attachment 770621 [details] [diff] [review]: ----------------------------------------------------------------- You've massively bitrotted a large patch of mine, but I'm not complaining!

https://hg.mozilla.org/mozilla-central/rev/9ed886b5122b https://hg.mozilla.org/mozilla-central/rev/0bd37861c48c https://hg.mozilla.org/mozilla-central/rev/13dd71c1b822 https://hg.mozilla.org/mozilla-central/rev/64cdd849567d https://hg.mozilla.org/mozilla-central/rev/909027ddcb43 https://hg.mozilla.org/mozilla-central/rev/fe791e969c9d https://hg.mozilla.org/mozilla-central/rev/d7ac4720b52b https://hg.mozilla.org/mozilla-central/rev/3a943844f56f https://hg.mozilla.org/mozilla-central/rev/18dfc741376f https://hg.mozilla.org/mozilla-central/rev/e4ee38bb3715

The forgotten part 12!

Parts 11--16: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f7ee0fd72e8 https://hg.mozilla.org/integration/mozilla-inbound/rev/dd66fa52787f https://hg.mozilla.org/integration/mozilla-inbound/rev/b77c33d1cb37 https://hg.mozilla.org/integration/mozilla-inbound/rev/010467c3da6e https://hg.mozilla.org/integration/mozilla-inbound/rev/551bd45d1b13 https://hg.mozilla.org/integration/mozilla-inbound/rev/298a680ab26b

https://hg.mozilla.org/mozilla-central/rev/6f7ee0fd72e8 https://hg.mozilla.org/mozilla-central/rev/dd66fa52787f https://hg.mozilla.org/mozilla-central/rev/b77c33d1cb37 https://hg.mozilla.org/mozilla-central/rev/010467c3da6e https://hg.mozilla.org/mozilla-central/rev/551bd45d1b13 https://hg.mozilla.org/mozilla-central/rev/298a680ab26b