1627954 - Record time taken by parallel GC tasks in the same place even if they were run on the main thread

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

Since bug 1592537 we can run parallel GC tasks on the main thread if no thread has stared the task by the time we start to wait for it, to avoid blocking GC completely if all threads are busy.

Currently this records the time taken by the task as a main thread GC phase, and I think this has resulted in some large movements in GC telemetry from what should be unrelated changes (telemetry for main thread phases and parallel task phases is reported separately)

The time taken by these tasks should always be attributed to parallel tasks, even if they end up being run on the main thread.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Evolution GC_SLOW_PHASE.png

Here's GC_SLOW_PHASE telemetry around the time bug 1618638 and then bug 1622757 landed. Phase 7 is unmarking, a parallel task, which suddenly showed up in the main thread telemetry, then disappeared again.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1627954 - Parallel task telemetry is sometimes misattributed r?sfink

Record all time taken by parallel tasks as parallel phases, even if they ended up being executed on the main thread.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d3f2162b557
Parallel task telemetry is sometimes misattributed r=sfink

Comment 4

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcea1326c9fb
Backed out changeset 8d3f2162b557 for causing multiple crashes on [@ js::gcstats::Statistics::lookupChildPhase(js::gcstats::PhaseKind) const]

Comment 5

[Arthur Iakab [arthur_iakab]]

Backed out changeset 8d3f2162b557 (bug 1627954) for causing multiple crashes on [@ js::gcstats::Statistics::lookupChildPhase(js::gcstats::PhaseKind) const]

Backout revision https://hg.mozilla.org/integration/autoland/rev/fcea1326c9fbac89229caaa38b65859126cd47ba

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8d3f2162b557eee29b56dba6774fdf02492e43f8&selectedJob=296766153

Failure logs https://treeherder.mozilla.org/logviewer.html#?job_id=296766153&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=296786403&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=296786505&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=296787755&repo=autoland

Jon can you please take a look?

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1627954 - Remove separate stats phase for gray unmarking that occurs during the course of normal marking r?sfink

This was causing problems for the previous patch because we don't collect timing information in phases for background tasks, and it's hard to tell when something is a background task or not when we can run them on the main thread.

This means that gray unmarking that occurs during the course of normal marking (the call to UnmarkGrayGCThingUnchecked in ShouldMarkCrossCompartment) no longer gets its own stats phase but I don't see that as much of an issue.

I tidied up a bit and used Maybe<> to replace the extra AutoPhase consturctor that took an 'enable' argument.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00843b46fca8
Remove separate stats phase for gray unmarking that occurs during the course of normal marking r=sfink
https://hg.mozilla.org/integration/autoland/rev/5f8c36c2bbbc
Parallel task telemetry is sometimes misattributed r=sfink

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/00843b46fca8
https://hg.mozilla.org/mozilla-central/rev/5f8c36c2bbbc