1225031 - Remove unused TypedArray generality

Status: RESOLVED FIXED

Description

[Lars T Hansen [:lth]]

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.

Comment 1

[Lars T Hansen [:lth]]

See bug 1176214 comment 36 for remarks by Waldo re what can/ought to be removed.

Comment 2

[Lars T Hansen [:lth]]

Created attachment 8702593 [details] [diff] [review]
get rid of the AnyTypedArray abstraction

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.

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bda46f8121539b2970666e61d609a8907300e59
Bug 1225031 - get rid of the AnyTypedArray abstraction. r=waldo

Comment 5

[Lars T Hansen [:lth]]

Leave open; still want to get rid of TypedArrayCommon.h.

Comment 6

[Carsten Book [:Tomcat]]

sorry had to backout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=19647984&repo=mozilla-inbound

Comment 7

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/151695836c37

Comment 8

[Lars T Hansen [:lth]]

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.

Comment 9

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d6dcb04a9d4

Comment 10

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a4595a79011dc47a6ac00eee59969586cccbe9
Bug 1225031 - get rid of the AnyTypedArray abstraction (updated).  r=waldo

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7a4595a7901

Comment 12

[André Bargull [:anba]]

Created attachment 8825945 [details] [diff] [review]
bug1225031-part1.patch

I've started working on this bug, but it's not yet completely finished. I'm attaching what I've got so far.

Comment 13

[André Bargull [:anba]]

Created attachment 8825946 [details] [diff] [review]
bug1225031-part2.patch

Comment 14

[André Bargull [:anba]]

Created attachment 8825947 [details] [diff] [review]
bug1225031-part3.patch

Comment 15

[André Bargull [:anba]]

Created attachment 8825948 [details] [diff] [review]
bug1225031-part4.patch

Comment 16

[André Bargull [:anba]]

Created attachment 8825949 [details] [diff] [review]
bug1225031-part5.patch

Comment 17

[André Bargull [:anba]]

Created attachment 8825950 [details] [diff] [review]
bug1225031-part6.patch

Comment 18

[André Bargull [:anba]]

Created attachment 8825951 [details] [diff] [review]
bug1225031-part7.patch

Comment 19

[André Bargull [:anba]]

Created attachment 8825952 [details] [diff] [review]
bug1225031-part8.patch

Comment 20

[André Bargull [:anba]]

Created attachment 8831081 [details] [diff] [review]
bug1225031-part9.patch

Comment 21

[Lars T Hansen [:lth]]

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?

Comment 22

[Lars T Hansen [:lth]]

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.

Comment 23

[Lars T Hansen [:lth]]

Comment on attachment 8825950 [details] [diff] [review]
bug1225031-part6.patch

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

Nice.

Comment 24

[Lars T Hansen [:lth]]

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.

Comment 25

[André Bargull [:anba]]

(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.

Comment 26

[Lars T Hansen [:lth]]

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.

Comment 27

[André Bargull [:anba]]

Created attachment 8834353 [details] [diff] [review]
bug1225031-part7.patch

Update part 7 to apply cleanly on inbound. Carrying r+ from lth.

Comment 28

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9252510307e2dca6e6f59201731594edf8251e0

Setting check-in needed for parts 1-9.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/2a59d5dff946
https://hg.mozilla.org/mozilla-central/rev/b02823469157
https://hg.mozilla.org/mozilla-central/rev/9fae70b0e004
https://hg.mozilla.org/mozilla-central/rev/6454a6b19d2e
https://hg.mozilla.org/mozilla-central/rev/8ca604105a21
https://hg.mozilla.org/mozilla-central/rev/64a8b9f67c2b
https://hg.mozilla.org/mozilla-central/rev/2db07fc0b189
https://hg.mozilla.org/mozilla-central/rev/2d1401b92435
https://hg.mozilla.org/mozilla-central/rev/75d9473b2744

Comment 31

[Lars T Hansen [:lth]]

André, any more work needed here that you're aware of?

Comment 32

[André Bargull [:anba]]

No, I don't think so.

Comment 33

[Firefox Product Integrity Bug Husbandry Bot (contact :emceeaich)]

Removing leave-open keyword from resolved bugs, per :sylvestre.