1552401 - When decommit runs on the main thread it is accounted in the compact phase.

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P2)

Description

[Paul Bone [:pbone]]

If a decommit thread should not be spawned or fails to spawn, then decommit is done in the foreground, but it is done in the compact phase and therefore reported incorrectly.

Comment 1

[Paul Bone [:pbone]]

Attachment: Bug 1552401 - Move main-thread decommit work into correct phase r=jonco

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #0)

Oh yes, I see, this does happen while the incremental state is State::Compact. But how does it get reported incorrectly? If it happens in the foreground it's not incremental and this shouldn't be observable.

Or is this on top of your incremental decommit changes?

Comment 3

[Paul Bone [:pbone]]

No, this is just on mozilla-central, no other changes.

It's only observable in things like the gecko profiler and aother things that report how much time is spent in each phase. It doesn't affect users at all.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #3)

I don't understand how the gecko profiler gets this information either.

Comment 5

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #4)

I don't understand how the gecko profiler gets this information either.

Through the use of the AutoPhase class in each step of incrementalSlice. It calls beginPhase() and endPhase() on the statistics object which records them. Later when generating the data this data is retrieved from the Statistics object and written out in JSON. The profiler readers some of the phase information as part of the marker tooltip.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #5)

Through the use of the AutoPhase class in each step of incrementalSlice.

Ah OK, but there's isn't an AutoPhase in use in the compact case in incrementalSlice. And the decommit case uses PhaseKind::WAIT_BACKGROUND_THREAD, which isn't what we want either.

Maybe you could add an AutoPhase to the startDecommit method instead? You'll need to add a stats phase in GeneratePhases.py.

Comment 7

[Paul Bone [:pbone]]

True. That's a simpler solution. I may use this code later to incrementalise on-main-thread decommit.

Comment 8

[Paul Bone [:pbone]]

Attachment: Bug 1552401 - Make GenerateStatsPhases.py print the next available bucket number r=jonco

Comment 9

[Paul Bone [:pbone]]

Attachment: Bug 1552401 - Add and use a new decommit phase to attribute decommit work r=jonco

Depends on D32089

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca8cc8b10cef
Make GenerateStatsPhases.py print the next available bucket number r=jonco
https://hg.mozilla.org/integration/autoland/rev/8ce4b2a4216c
Add and use a new decommit phase to attribute decommit work r=jonco

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/ca8cc8b10cef
https://hg.mozilla.org/mozilla-central/rev/8ce4b2a4216c