Closed Bug 1599416 Opened 1 year ago Closed 1 year ago

Remove unused TypedObject functions

Categories

(Core :: JavaScript Engine, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla72
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.

Entries are never added to lazyArrayBuffers resp. lazyArrayBuffers isn't
even allocated anymore.

Depends on D54710

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

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

  • Instead of testing for InlineTransparentTypedObject and InlineOpaqueTypedObject
    separately, we can directly test for InlineTypedObject.
  • Because the ArrayBuffer of an OutlineTypedObject can never be detached, we
    don't need to test for it.

Depends on D54713

The length parameter is always 1.

Depends on D54714

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

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

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

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

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.

I understand, which is why I didn't say "don't do this" :-) Just be careful where you cut, is all.

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.

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
You need to log in before you can comment on or make changes to this bug.