Remove unused TypedArray generality

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: lth, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 1 obsolete attachment)

The introduction of SharedTypedArray caused some TypedArray functionality to be generalized, giving rise to functions such as IsAnyTypedArray and AnyTypedArrayLength, as well as to the file TypedArrayCommon.h and the class TypedArrayLayout.

With bug 1176214, SharedTypedArray is being removed and it takes TypedArrayLayout with it, but does not do anything about the 'generic' TypedArray functions or TypedArrayCommon.h (or anything else I can't think of right now).  We should clean this up.
See bug 1176214 comment 36 for remarks by Waldo re what can/ought to be removed.
Depends on: 1055472
Another step in getting rid of TypedArrayCommon.h.  I beg the court's indulgence over the currently overlong lines at the end of that file, which have only gotten longer with this change.
Attachment #8702593 - Flags: review?(jwalden+bmo)
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Comment on attachment 8702593 [details] [diff] [review]
get rid of the AnyTypedArray abstraction

Review of attachment 8702593 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MIR.cpp
@@ +5000,2 @@
>                                    MDefinition* obj, MDefinition* id,
>                                    Scalar::Type* arrayType)

Realign.

::: js/src/vm/TypedArrayCommon.h
@@ +162,2 @@
>                           Handle<SomeTypedArray*> target, HandleObject source,
>                           uint32_t offset)

Realign the parameters here, please.
Attachment #8702593 - Flags: review?(jwalden+bmo) → review+
Leave open; still want to get rid of TypedArrayCommon.h.
Keywords: leave-open
A bug in ElementSpecific::setFromTypedArray() that came in with this patch -- podCopy<T> was changed to podCopy<uint8> without multiplying the element count by sizeof(T), but it's really the cast that's wrong, I think, we can just use T.  Will do a try run.
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
I've started working on this bug, but it's not yet completely finished. I'm attaching what I've got so far.
Posted patch bug1225031-part7.patch (obsolete) — Splinter Review
Attachment #8825945 - Flags: review?(lhansen)
Attachment #8825946 - Flags: review?(lhansen)
Attachment #8825947 - Flags: review?(lhansen)
Attachment #8825948 - Flags: review?(lhansen)
Attachment #8825949 - Flags: review?(lhansen)
Attachment #8825950 - Flags: review?(lhansen)
Attachment #8825951 - Flags: review?(lhansen)
Attachment #8825952 - Flags: review?(lhansen)
Attachment #8831081 - Flags: review?(lhansen)
Comment on attachment 8825945 [details] [diff] [review]
bug1225031-part1.patch

Review of attachment 8825945 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/TypedArrayCommon.h
@@ +483,5 @@
>                                   uint32_t offset)
>      {
>          MOZ_ASSERT(SpecificArray::ArrayTypeID() == target->type(),
>                     "calling wrong setFromTypedArray specialization");
> +        MOZ_ASSERT(TypedArrayObject::sameBuffer(target, source),

Memo to self: does sameBuffer() check whether, if two objects are different, they may still be shared objects with the same memory?
Attachment #8825945 - Flags: review?(lhansen) → review+
Attachment #8825948 - Flags: review?(lhansen) → review+
Attachment #8825949 - Flags: review?(lhansen) → review+
Attachment #8825951 - Flags: review?(lhansen) → review+
Comment on attachment 8825952 [details] [diff] [review]
bug1225031-part8.patch

Review of attachment 8825952 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/TypedArrayObject.cpp
@@ +8,1 @@
>  #include "vm/TypedArrayObject.h"

Hm, I seriously doubt that.  Normally inlines are included later.  But that can be between you and the Sheriff.
Attachment #8825952 - Flags: review?(lhansen) → review+
Attachment #8825946 - Flags: review?(lhansen) → review+
Attachment #8825947 - Flags: review?(lhansen) → review+
Comment on attachment 8825950 [details] [diff] [review]
bug1225031-part6.patch

Review of attachment 8825950 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8825950 - Flags: review?(lhansen) → review+
Attachment #8831081 - Flags: review?(lhansen) → review+
Nice work.

For future reference: Generally speaking I prefer that each patch (a) has a title that indicative of what it does, for review-mnemonic purposes, and (b) is submitted with a comment that outlines what the patch does and why.  I do not believe that code speaks for itself.
(In reply to Lars T Hansen [:lth] from comment #24)
> For future reference: Generally speaking I prefer that each patch (a) has a
> title that indicative of what it does, for review-mnemonic purposes, and (b)
> is submitted with a comment that outlines what the patch does and why.  I do
> not believe that code speaks for itself.

(Most of the time) I write a small comment for review patches, especially when it's not obvious why a change is needed. For example part 7 seemingly adds random #includes to different files, whereas in a proper review comment I'd have explained why this was necessary. So why didn't I write any review comments at all for these patches? Well, I didn't plan to work on this bug any time soon, but instead only wanted to back-up the current work by uploading it to BMO. Except that not "any time soon" turned out to be only two weeks, because then I started to work on bug 1317384. So I made not one, but two mistakes: 1. I should have added comments when I uploaded the patches, even when I didn't plan to work on them. 2. And when I changed my mind to work on this bug, I shouldn't have requested review without previously updating the patches to add comments. 
So, if you ever again come across a patch from me and you don't feel comfortable reviewing the patch, e.g. because it lacks comments, please let me know and simply refuse reviewing it until I've fixed it to be in a reviewable shape.
I'm generally pretty good about asking for information if I need it  :-)  And when I know the code as well as I do this code it's not a big deal.  I hope what I wrote wasn't too strongly worded; it was just intended as a gentle nudge.
Update part 7 to apply cleanly on inbound. Carrying r+ from lth.
Attachment #8825951 - Attachment is obsolete: true
Attachment #8834353 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a59d5dff946
Part 1: Directly use TypedArrayObject in ElementSpecific and remove no longer used typedefs. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/b02823469157
Part 2: Replace typedarray template parameter with template parameter over storage type. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fae70b0e004
Part 3: Remove template parameter from TypedArrayMethods. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6454a6b19d2e
Part 4: Use explicit types for parameters instead of relying on runtime assertions. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ca604105a21
Part 5: Move TypedArray.prototype.set from TypedArrayCommon to TypedArrayObject.cpp. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a8b9f67c2b
Part 6: Remove switch over TypedArray's underlying type from TypedArrayMethods. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2db07fc0b189
Part 7: Replace includes for TypedArrayCommon.h with TypedArrayObject.h. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1401b92435
Part 8: Rename TypedArrayCommon.h to TypedArrayObject-inl.h. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/75d9473b2744
Part 9: Inline methods from TypedArrayMethods and then remove TypedArrayMethods. r=lth
Keywords: checkin-needed
André, any more work needed here that you're aware of?
Flags: needinfo?(andrebargull)
No, I don't think so.
Flags: needinfo?(andrebargull)
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.