Remove unused TypedArray generality

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: lth, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
See bug 1176214 comment 36 for remarks by Waldo re what can/ought to be removed.
(Reporter)

Updated

3 years ago
Depends on: 1055472
(Reporter)

Comment 2

2 years ago
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.
Attachment #8702593 - Flags: review?(jwalden+bmo)
(Reporter)

Updated

2 years ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED

Comment 3

2 years ago
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+
(Reporter)

Comment 5

2 years ago
Leave open; still want to get rid of TypedArrayCommon.h.
Keywords: leave-open
(Reporter)

Comment 8

2 years ago
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.
(Reporter)

Comment 10

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

Updated

2 years ago
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
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.
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)
Created attachment 8831081 [details] [diff] [review]
bug1225031-part9.patch
Attachment #8831081 - Flags: review?(lhansen)
(Reporter)

Comment 21

a year ago
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+
(Reporter)

Updated

a year ago
Attachment #8825948 - Flags: review?(lhansen) → review+
(Reporter)

Updated

a year ago
Attachment #8825949 - Flags: review?(lhansen) → review+
(Reporter)

Updated

a year ago
Attachment #8825951 - Flags: review?(lhansen) → review+
(Reporter)

Comment 22

a year ago
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+
(Reporter)

Updated

a year ago
Attachment #8825946 - Flags: review?(lhansen) → review+
(Reporter)

Updated

a year ago
Attachment #8825947 - Flags: review?(lhansen) → review+
(Reporter)

Comment 23

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

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

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

Updated

a year ago
Attachment #8831081 - Flags: review?(lhansen) → review+
(Reporter)

Comment 24

a year ago
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.
(Reporter)

Comment 26

a year ago
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.
Created attachment 8834353 [details] [diff] [review]
bug1225031-part7.patch

Update part 7 to apply cleanly on inbound. Carrying r+ from lth.
Attachment #8825951 - Attachment is obsolete: true
Attachment #8834353 - Flags: review+
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9252510307e2dca6e6f59201731594edf8251e0

Setting check-in needed for parts 1-9.
Keywords: checkin-needed

Comment 29

a year ago
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
(Reporter)

Comment 31

a year ago
André, any more work needed here that you're aware of?
Flags: needinfo?(andrebargull)
No, I don't think so.
Flags: needinfo?(andrebargull)
(Reporter)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year 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.