1785804 - Consider removing DoMarking fast path in tracer dispatch

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

Tracing GC things in SpiderMonkey works via virtual method dispatch, except for a special fast path for marking:

https://searchfox.org/mozilla-central/source/js/src/gc/Marking.cpp#724-727

Removing this would simplify the code and make it easier to follow by making marking just another kind of tracing.

It's possible that this will regress performance. However marking also has special cases for eager marking of non-cyclic GC things and for marking objects and these should end up being used most of the time.

Eventually we should be able to get the compiler to devirtualise these calls, which would mitigate any performance impact.

Comment 1

[Jon Coppeard (:jonco)]

To make things clear, this is about removing the DoMarking fast path, not the GCMarker::eagerlyMarkChildren fast path, which will be investigated in a later bug.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1785804 - Part 0: Remove unused enum values and methods in TracingAPI.h r?sfink

---

imported patch remove-unused-gc-number-method

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1785804 - Part 1: Move GC thing trace methods to an inline header file r?sfink

This moves all GC thing trace methods (and optimised GCMarker versions that do
eager marking) to their own header. With later patches this should allow
devirtualisation of the tracer dispatch.

A follow on bug will attempt to remove some of the GCMarker versions.

Depends on D156558

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1785804 - Part 2: Remove tracing name context and pass name through the trace methods r?sfink

One point of difference between the DoMarking fast path and other tracers is
handling the edge name. Passing this through to the concrete tracer and
handling it there removes this differemce.

Eventually we should be able to inline some of these these calls at which point
it should get optimsed away for tracers that ignore it.

Depends on D156559

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1785804 - Part 3: Pass edge pointer to trace methods r?sfink

Similar to the previous patch, this delegates whether to update the edge to the
implementation of the trace method, so we don't need generic tracing code to
handle that possiblity any more.

Depends on D156560

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1785804 - Part 4: Remove DoMarking fast path and make GCMarker a GenericTracer r?sfink

This removes the fast path so we use virtual dispatch for all tracers.

Depends on D156561

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1785804 - Part 5: Merge GenericTracer into JSTracer r?sfink

All tracers are now GenericTracers so there's no reason for this to be a
separate thing.

Depends on D156562

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1785804 - Part 6: Inline TraceEdge methods for non-tagged pointers r?sfink

For non-tagged pointer types, DoCallback is now simple enough that we can
define this inline in Tracing.h and most TraceEdge methods become a virtual
method call.

Depends on D156563

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55855c5ad184
Part 0: Remove unused enum values and methods in TracingAPI.h r=sfink
https://hg.mozilla.org/integration/autoland/rev/271975a555d0
Part 1: Move GC thing trace methods to an inline header file r=sfink
https://hg.mozilla.org/integration/autoland/rev/a115e9eaffee
Part 2: Remove tracing name context and pass name through the trace methods r=sfink
https://hg.mozilla.org/integration/autoland/rev/f9e864cd34a6
Part 3: Pass edge pointer to trace methods r=sfink
https://hg.mozilla.org/integration/autoland/rev/27771c8d9865
Part 4: Remove DoMarking fast path and make GCMarker a GenericTracer r=sfink
https://hg.mozilla.org/integration/autoland/rev/361dc2862d74
Part 5: Merge GenericTracer into JSTracer r=sfink
https://hg.mozilla.org/integration/autoland/rev/31ef32f519f2
Part 6: Inline TraceEdge methods for non-tagged pointers r=sfink

Comment 10

[Atila Butkovits]

Backed outfor causing reftest crashes at @ mozilla::dom::Promise::MaybeReject(nsresult).

Backout link: https://hg.mozilla.org/integration/autoland/rev/fc02f1e9c4843648b6791d4217d124fae5f986d9

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=RaFfmy_zR9KYenELeEpYGg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=31ef32f519f2b306e331cfb420d8d4f9128cdeea

Failure log: https://treeherder.mozilla.org/logviewer?job_id=389722922&repo=autoland&lineNumber=52981

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e226dbfe8fa
Part 0: Remove unused enum values and methods in TracingAPI.h r=sfink
https://hg.mozilla.org/integration/autoland/rev/6897324c32c5
Part 1: Move GC thing trace methods to an inline header file r=sfink
https://hg.mozilla.org/integration/autoland/rev/88c46af9c2c5
Part 2: Remove tracing name context and pass name through the trace methods r=sfink
https://hg.mozilla.org/integration/autoland/rev/1fc5ea164fd7
Part 3: Pass edge pointer to trace methods r=sfink
https://hg.mozilla.org/integration/autoland/rev/5aaa5504196b
Part 4: Remove DoMarking fast path and make GCMarker a GenericTracer r=sfink
https://hg.mozilla.org/integration/autoland/rev/2b2a03b16808
Part 5: Merge GenericTracer into JSTracer r=sfink
https://hg.mozilla.org/integration/autoland/rev/81fd7885f181
Part 6: Inline TraceEdge methods for non-tagged pointers r=sfink

Comment 12

[Atila Butkovits]

Relanded because the real issue was from a different bug.

Comment 13

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/1e226dbfe8fa
https://hg.mozilla.org/mozilla-central/rev/6897324c32c5
https://hg.mozilla.org/mozilla-central/rev/88c46af9c2c5
https://hg.mozilla.org/mozilla-central/rev/1fc5ea164fd7
https://hg.mozilla.org/mozilla-central/rev/5aaa5504196b
https://hg.mozilla.org/mozilla-central/rev/2b2a03b16808
https://hg.mozilla.org/mozilla-central/rev/81fd7885f181