Closed
Bug 1225031
Opened 6 years ago
Closed 4 years ago
Remove unused TypedArray generality
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: lth, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(10 files, 1 obsolete file)
66.47 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
10.79 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
23.83 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
6.41 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
6.31 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
6.45 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
15.89 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
3.80 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
7.57 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
10.40 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
See bug 1176214 comment 36 for remarks by Waldo re what can/ought to be removed.
Reporter | ||
Comment 2•5 years ago
|
||
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•5 years ago
|
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Comment 3•5 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 4•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bda46f8121539b2970666e61d609a8907300e59 Bug 1225031 - get rid of the AnyTypedArray abstraction. r=waldo
Reporter | ||
Comment 5•5 years ago
|
||
Leave open; still want to get rid of TypedArrayCommon.h.
Keywords: leave-open
Comment 6•5 years ago
|
||
sorry had to backout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=19647984&repo=mozilla-inbound
Reporter | ||
Comment 8•5 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 9•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d6dcb04a9d4
Reporter | ||
Comment 10•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a4595a79011dc47a6ac00eee59969586cccbe9 Bug 1225031 - get rid of the AnyTypedArray abstraction (updated). r=waldo
Comment 11•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7a4595a7901
Reporter | ||
Updated•5 years ago
|
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
Comment 12•4 years ago
|
||
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•4 years ago
|
||
Comment 14•4 years ago
|
||
Comment 15•4 years ago
|
||
Comment 16•4 years ago
|
||
Comment 17•4 years ago
|
||
Comment 18•4 years ago
|
||
Comment 19•4 years ago
|
||
Updated•4 years ago
|
Attachment #8825945 -
Flags: review?(lhansen)
Updated•4 years ago
|
Attachment #8825946 -
Flags: review?(lhansen)
Updated•4 years ago
|
Attachment #8825947 -
Flags: review?(lhansen)
Updated•4 years ago
|
Attachment #8825948 -
Flags: review?(lhansen)
Updated•4 years ago
|
Attachment #8825949 -
Flags: review?(lhansen)
Updated•4 years ago
|
Attachment #8825950 -
Flags: review?(lhansen)
Updated•4 years ago
|
Attachment #8825951 -
Flags: review?(lhansen)
Updated•4 years ago
|
Attachment #8825952 -
Flags: review?(lhansen)
Comment 20•4 years ago
|
||
Attachment #8831081 -
Flags: review?(lhansen)
Reporter | ||
Comment 21•4 years 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•4 years ago
|
Attachment #8825948 -
Flags: review?(lhansen) → review+
Reporter | ||
Updated•4 years ago
|
Attachment #8825949 -
Flags: review?(lhansen) → review+
Reporter | ||
Updated•4 years ago
|
Attachment #8825951 -
Flags: review?(lhansen) → review+
Reporter | ||
Comment 22•4 years 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•4 years ago
|
Attachment #8825946 -
Flags: review?(lhansen) → review+
Reporter | ||
Updated•4 years ago
|
Attachment #8825947 -
Flags: review?(lhansen) → review+
Reporter | ||
Comment 23•4 years 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•4 years ago
|
Attachment #8831081 -
Flags: review?(lhansen) → review+
Reporter | ||
Comment 24•4 years 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.
Comment 25•4 years ago
|
||
(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•4 years 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.
Comment 27•4 years ago
|
||
Update part 7 to apply cleanly on inbound. Carrying r+ from lth.
Attachment #8825951 -
Attachment is obsolete: true
Attachment #8834353 -
Flags: review+
Comment 28•4 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9252510307e2dca6e6f59201731594edf8251e0 Setting check-in needed for parts 1-9.
Keywords: checkin-needed
Comment 29•4 years 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
Comment 30•4 years ago
|
||
bugherder |
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
Reporter | ||
Comment 31•4 years ago
|
||
André, any more work needed here that you're aware of?
Flags: needinfo?(andrebargull)
Reporter | ||
Updated•4 years ago
|
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment 33•3 years ago
|
||
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.
Description
•