Closed Bug 1668825 Opened 4 years ago Closed 4 years ago

Refactor public tracing interface to remove details of heap storage

Categories

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

task

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox83 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(13 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

Right now the heap storage is part of the tracing interface because our |onFooEdge| virtual methods take a Foo**. If we want to try and make heap storage atomic (for concurrent marking) then that will have to change this interface. It would be preferable to keep these details out of public interface by only exposing the |onChild| interface, which is the only part that gets used outside SpiderMonkey anyway.

Keywords: leave-open

The instances of this are called 'actions' everywhere and I think it makes
sense that the type should be called that. Also make it an enum class and move
it into the JS namespace.

Currently we have two different enums that determine the tracer kind. Combine
this into a single enum and move it to the JS namespace.

Depends on D92252

This splits out a GenericTracer class (with a virtual method per edge kind)
from CallbackTracer (with a single virtual method for all edges). GenericTracer
is used interally whereas all external uses use CallbackTracer.

Depends on D92253

This gives JSTracer a nullable pointer to a JS::TracingContext. This will only be set for CallbackTracers.

Depends on D92255

This isn't used anywhere so we can make onChild void.

Depends on D92256

Existing uses changed to construct and GCCellPtr and call JS::TraceChildren instead.

Depends on D92257

Attachment #9179296 - Attachment description: Bug 1668825 - Combine TracerKindTag and TracerKind r?sfink → Bug 1668825 - Combine TracerKindTag and TracerKind r=sfink! r=mccr8!
Attachment #9179296 - Attachment description: Bug 1668825 - Combine TracerKindTag and TracerKind r=sfink! r=mccr8! → Bug 1668825 - Combine TracerKindTag and TracerKind r=sfink!,mccr8!
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac952bad0e4e
Rename WeakMapTraceKind to WeakMapTraceAction and make it an enum class r=sfink
https://hg.mozilla.org/integration/autoland/rev/862bae6bac69
Combine TracerKindTag and TracerKind r=sfink
https://hg.mozilla.org/integration/autoland/rev/7cfb76e5d6ba
Split CallbackTracer into GenericTracer and CallbackTracer r=sfink
https://hg.mozilla.org/integration/autoland/rev/940c9b71f178
Move canSkipJsid to JSTracer so it's together with the other tracing options r=sfink
https://hg.mozilla.org/integration/autoland/rev/52e9a1f9b9c1
Split out tracing context information into a separate class r=sfink
https://hg.mozilla.org/integration/autoland/rev/ea033bf01dd0
Remove unused onChild return value r=sfink
https://hg.mozilla.org/integration/autoland/rev/fef8db29083e
Remove deprectated js::TraceChildren function that takes separate pointer and trace kind r=sfink
https://hg.mozilla.org/integration/autoland/rev/6ae7a5b34ffb
Remove JS_GetTraceThingInfo from the public API since it's only used internally r=sfink

I gave TraceOptions some implicit consturctors to make it easier to set a
single option by passing an enum value.

Depends on D92595

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e6a88c27779
Remove JSTracer's checkEdges option which doesn't seem to be necessary any more r=sfink
https://hg.mozilla.org/integration/autoland/rev/5bc8cb307c61
Move trace options into a separate options class passed when the tracer is created r=sfink
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/118d9d444b76
Remove JSTracer's checkEdges option which doesn't seem to be necessary any more r=sfink
https://hg.mozilla.org/integration/autoland/rev/a6331448cef2
Move trace options into a separate options class passed when the tracer is created r=sfink

These don't need to be in the public header. We can do this with static functions.

This API is only used in one place, with a callback that calls into a tracer,
so we can make this take a tracer in the first place and cut out one level of
indirection.

Depends on D93152

Depends on: 1670251
Flags: needinfo?(jcoppeard)
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac6536591c1c
Move disptachToOnEdge methods out of class definition r=sfink
https://hg.mozilla.org/integration/autoland/rev/a90ba60415cc
Change VisitGrayWrapperTargets API to take a tracer rather than a callback r=sfink

The final patch in this series. This changes the GenericTracer APIs to take a
thing pointer and return a possibly updated versions, rather than taking a
double pointer to the thing. This means that we can change the details of how
pointers are stored in the heap without chaning this interface.

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1547b1a71895
Change GenericTracer API so that it doesn't include details of heap storage r=sfink
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: