1670251 - GCManagedDeletePolicy is obscure and doesn't work for GCPtr<Value>

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

There's two problems here: the first is that it's not obvious that you need to use GCManagedDeletePolicy, and the second that is that it doesn't work in all cases (although the failure case doesn't arise at the moment).

To recap, this is used for classes containing GCPtr that need to be deleted outside of GC. GCPtr is mean to be used inside classes that have GC lifetime, but sometimes you do need to delete them (e.g. if you OOM while halfway through initialisation).

It would be better to move away from using this where possible. HeapPtr is always correct to use, but there might be a performance cost as this needs to check whether barriers are required (barriers are not required during GC finalization so GCPtr skips these).

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1670251 - Only release empty areas after everything has been finalized r?sfink

This makes the HeapPtr destructor work with pointers between cells with
different alloc kinds, specifially from background to foreground finalized
ones. Currently this doesn't work because the arena can have been released by
the time the background finalized HeapPtr runs, so it can't check the referent
to know whether it needs to peform a write barrier.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1670251 - Replace GCPtr with HeapPtr in data structures that can be destroyed outside of GC sweeping r?sfink

This means we don't need to use GCManagedDeletePolicy for these any more. I
don't think this will have a performance impact but it's possible.

It would be possible to add a TenuredHeapPtr wrapper for use in places where
you have a pointer kinds that are nursery allocatable but not used for nursery
pointers in practice. That could help mitigate any perf impact caused by the
scope changes.

Depends on D93162

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1670251 - Remove JSScript from GCManagedDeletePolicy r?sfink

This wasn't actually necessary since PrivateScriptData doesn't use GCPtr and
the barrier is done manually in BaseScript::swapData already. This class is
probably pretty fragile from a GC point of view but we'll live with that I
guess.

Depends on D93163

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1670251 - Disallow GCPtr destruction outside GC finalization and remove GCManagedDeletePolicy r?sfink

Depends on D93164

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31f56067f538
Only release empty areas after everything has been finalized r=sfink
https://hg.mozilla.org/integration/autoland/rev/9f69c2835688
Replace GCPtr with HeapPtr in data structures that can be destroyed outside of GC sweeping r=sfink
https://hg.mozilla.org/integration/autoland/rev/969b0b711982
Remove JSScript from GCManagedDeletePolicy r=sfink
https://hg.mozilla.org/integration/autoland/rev/97770f1880a2
Disallow GCPtr destruction outside GC finalization and remove GCManagedDeletePolicy r=sfink

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/31f56067f538
https://hg.mozilla.org/mozilla-central/rev/9f69c2835688
https://hg.mozilla.org/mozilla-central/rev/969b0b711982
https://hg.mozilla.org/mozilla-central/rev/97770f1880a2