Remove unused TypedObject functions
Categories
(Core :: JavaScript Engine, task)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox72 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(14 files)
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review |
There's a bunch of TypedObject functions which aren't used anymore.
|
Assignee | |
Comment 1•5 years ago
|
||
|
|
Assignee | |
Comment 2•5 years ago
|
||
Depends on D54705
|
|
Assignee | |
Comment 3•5 years ago
|
||
Depends on D54706
|
|
Assignee | |
Comment 4•5 years ago
|
||
Depends on D54707
|
|
Assignee | |
Comment 5•5 years ago
|
||
Depends on D54708
|
|
Assignee | |
Comment 6•5 years ago
|
||
Depends on D54709
|
|
Assignee | |
Comment 7•5 years ago
|
||
Entries are never added to lazyArrayBuffers resp. lazyArrayBuffers isn't
even allocated anymore.
Depends on D54710
|
|
Assignee | |
Comment 8•5 years ago
|
||
ArrayBufferObject::detach expects all views are ArrayBufferViewObject, which
means detaching an ArrayBuffer used for TypedObjects already crashes anyway.
Instead let's change ArrayBufferObject::addView and ABO::setFirstView to only
accept ArrayBufferViewObject and then remove the addView call in
OutlineTypedObject::attach. Additionally introduce ABO::createForTypedObject
as the single function which can call ABO::setHasTypedObjectViews to mark an
ArrayBufferObject as being used for TypedObjects.
Depends on D54711
|
|
Assignee | |
Comment 9•5 years ago
|
||
Because ArrayBuffers used for TypedObjects can never be detached, we can remove
a couple of additional functions which are no longer used.
Depends on D54712
|
|
Assignee | |
Comment 10•5 years ago
|
||
- Instead of testing for
InlineTransparentTypedObjectandInlineOpaqueTypedObject
separately, we can directly test forInlineTypedObject. - Because the ArrayBuffer of an
OutlineTypedObjectcan never be detached, we
don't need to test for it.
Depends on D54713
|
|
Assignee | |
Comment 11•5 years ago
|
||
The length parameter is always 1.
Depends on D54714
|
|
Assignee | |
Comment 12•5 years ago
|
||
AttacheTypedObject is only ever called directly after NewOpaqueTypedObject,
so we can merge both functions similar to the existing NewDerivedTypedObject
function.
This is in preparation of the next two patches.
Depends on D54715
|
|
Assignee | |
Comment 13•5 years ago
|
||
OutlineTypedObject::attach is only called when creating a new OutlineTypedObject.
By making the two attach functions and createUnattached[WithClass] private
methods, we can enforce only OutlineTypedObjects with attached data are exposed
to the user.
This is in preparation for the next patch.
Depends on D54716
|
|
Assignee | |
Comment 14•5 years ago
|
||
As demonstrated in the last patch, OutlineTypedObjects always have an attached
datum, which means TypedObject::isAttached always returns true for any
TypedObject.
The two new assertions in OutlineTypedObject::obj_trace have been added so
it's easier to see that owner_ is nullptr iff data_ is nullptr.
Depends on D54717
|
||
Comment 15•5 years ago
|
||
+1 for cleanup but this makes me slightly nervous. We will probably end up using TypedObjects in some form for the wasm "GC" feature. I have no idea what we're going to need, so it's good if you can have a light touch here.
|
|
Assignee | |
Comment 16•5 years ago
|
||
I know that there are plans to reuse TypedObjects for wasm, the thing is the code currently present in the code base isn't tested anymore and is partially broken. For example large parts of these clean-up patches are around TypedObjects using detached ArrayBuffers (this functionality was removed in early summer 2018 and the still present parts got broken over time), but when actually trying to detach an ArrayBuffer which is used for TypedObjects, you'll get a crash. So I'm not sure it's worthwhile to keep this code, because it gives the false impression it still works, which it clearly doesn't. Well, and in the worst case we can hg revert these patches if we end up needing (parts of) them again for wasm.
|
|
||
Comment 17•5 years ago
|
||
I understand, which is why I didn't say "don't do this" :-) Just be careful where you cut, is all.
|
||
Comment 18•5 years ago
|
||
My feeling in favour of landing these patches is that they're really clean cuts, and where wasm GC needs them, I suspect they could be piecemeal reverted or re-implmented to better effect.
|
||
Comment 19•5 years ago
|
||
Pushed by aciure@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1f76130b2cae Part 1: Remove unused OutlineTypedObject::notifyBufferDetached method. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/ade7361b947c Part 2: Remove unused SetTypedObjectOffset function. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/ca3152f19e29 Part 3: Remove unused ObjectIsOpaqueTypedObject function. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/10f7acca7244 Part 4: Remove unused ObjectIsTransparentTypedObject function. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/c49111d0a440 Part 5: Remove unused TypedObject::GetByteOffset function. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/8c36a33102d5 Part 6: Remove unused TypedObject functions. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/fc9e0fa219ac Part 7: Remove unused lazyArrayBuffers table. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/17f3a6b3c21f Part 8: Remove view tracking for TypedObjects. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/039c220b5666 Part 9: Remove tracking for TypedObjects with detached buffers. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/f98095b88847 Part 10: Simplify TypedObject::isAttached. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/27b601310b36 Part 11: Remove constant parameters. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/12b167e83edd Part 12: Fold AttachTypedObject into NewOpaqueTypedObject. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/8fee074100ca Part 13: Make OutlineTypedObject::attach and createUnattached private methods. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/7a5a3cbc9b5b Part 14: Remove TypedObject::isAttached because it always returns true. r=mgaudet
|
||
Comment 20•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/1f76130b2cae
https://hg.mozilla.org/mozilla-central/rev/ade7361b947c
https://hg.mozilla.org/mozilla-central/rev/ca3152f19e29
https://hg.mozilla.org/mozilla-central/rev/10f7acca7244
https://hg.mozilla.org/mozilla-central/rev/c49111d0a440
https://hg.mozilla.org/mozilla-central/rev/8c36a33102d5
https://hg.mozilla.org/mozilla-central/rev/fc9e0fa219ac
https://hg.mozilla.org/mozilla-central/rev/17f3a6b3c21f
https://hg.mozilla.org/mozilla-central/rev/039c220b5666
https://hg.mozilla.org/mozilla-central/rev/f98095b88847
https://hg.mozilla.org/mozilla-central/rev/27b601310b36
https://hg.mozilla.org/mozilla-central/rev/12b167e83edd
https://hg.mozilla.org/mozilla-central/rev/8fee074100ca
https://hg.mozilla.org/mozilla-central/rev/7a5a3cbc9b5b
Description
•