1789425 - Verify if we can interrupt "forget skippable" if mozilla::ipc::ProcessChild::ExpectingShutdown()

Status: NEW

(Core :: XPCOM, task)

Description

[Jens Stutte [:jstutte]]

Example: https://crash-stats.mozilla.org/report/index/47808298-3e70-4768-a5aa-7c9d80220906

Comment 1

[Jon Coppeard (:jonco)]

(In reply to Jens Stutte [:jstutte] from comment #0)

Example: https://crash-stats.mozilla.org/report/index/47808298-3e70-4768-a5aa-7c9d80220906

I'm not sure what's being suggested here, but this is in the CC.

Comment 2

[Jens Stutte [:jstutte]]

Then maybe Olli has a better suggestion for what to do.

Comment 3

[Andrew McCreight [:mccr8]]

This is actually forget skippable, which runs outside of the CC. Maybe we need to check if shutdown has started before we do that? I know we have checks like that in a few places but maybe not for forget skippable?

Comment 4

[Steve Fink [:sfink] [:s:]]

There is a check for shutdown, but it only checks whether shutdown is already happening. It looks like this would need to check whether there is an impending shutdown, and I'm not sure what the right way to do that would be with the current layering.

ProcessChild could expose a public accessor to sExpectingShutdown, I guess? I don't know if it's ok for nsJSEnvironment.cpp or CCGCScheduler.h to access that.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

sfink answered here already :)

Comment 6

[Jens Stutte [:jstutte]]

(In reply to Andrew McCreight [:mccr8] from comment #3)

This is actually forget skippable, which runs outside of the CC. Maybe we need to check if shutdown has started before we do that? I know we have checks like that in a few places but maybe not for forget skippable?

My assumption is that at least in the example from comment 0 the runnable is already executed when NotifiedImpendingShutdown is called on the IPC I/O thread, otherwise we should probably see the RecvShutdownConfirmedHP annpotation in IPC shutdown state (as that is scheduled with high priority). I thus doubt that checking before we start to run this is enough and that we should have something similar like we have for JS (see bug 1777198) to interrupt while running (note to myself that we still need to flip that pref for release). But there might be also other ways to shorten this runnables' runtime, like checking for how long we already ran? After all it sounds bad to potentially block the main thread such long for an idle task not only for the shutdown case.

(In reply to Steve Fink [:sfink] [:s:] from comment #4)

ProcessChild could expose a public accessor to sExpectingShutdown, I guess? I don't know if it's ok for nsJSEnvironment.cpp or CCGCScheduler.h to access that.

We already have mozilla::ipc::ProcessChild::ExpectingShutdown(). The underlying sExpectingShutdown is atomic (and it is ever set only in child processes). So technically it would work to access it from everywhere inside our process, not sure if it is sound. We currently use it in the process hang monitor and to cancel synchronous local storage requests.

Comment 7

[Jens Stutte [:jstutte]]

Just to be clear: it is understood that there might be situations where the OS is just unresponsive. In some of these cases we could see some heavy delay from memory that must be swapped in - just to be freed right afterwards. Assuming that we are looping here over a set of objects that are potentially sparsely distributed in memory (and thus will most probably have been swapped out into different blocks of the swap file), breaking that loop under certain conditions (shutdown or unexpectedly long execution time) might help to some extent (if thanks to that we are able to just close the process right away without swapping in all the memory).

Obviously I did not check and ignore if we have already any measures of that sort (and there might be more than one loop in different places to which this could apply).

Comment 8

[Jens Stutte [:jstutte]]

Can I assume that https://crash-stats.mozilla.org/report/index/8df6bbb4-eb7a-4e9e-b318-f46710220908 is a different case (in the sense of running a real JS GC: nsJSContext::RunIncrementalGCSlice) ? If we want to interrupt JS - would we also want to interrupt JS GC (independently from shutdown, which just requests an interrupt, too) ?

Comment 9

[Andrew McCreight [:mccr8]]

Yes, that would be different. It looks like bug 1672121 has some recent-ish experiment with interrupting GC.

Comment 10

[Steve Fink [:sfink] [:s:]]

Yes, GC is interruptible, and that mechanism could be adapted to work here. Right now, it is only interrupted if a callback is triggered from TaskController::MaybeInterruptTask, which is based on task priorities. Setting an atomic wouldn't do that, but we could still make the budget register a separate atomic to check or something. Alternatively, ProcessChild could allow registering a callback to be invoked in ProcessChild::NotifiedImpendingShutdown that would then invoke RequestInterrupt. I think both work, it's just a matter of where we want to add the complexity.

What I'm unsure about is whether it's ok to skip the scan here. Obviously, it's fine to abort ForgetSkippable, since that's an optimization. But are we going to need to do the CC before shutdown anyway, so this would purely be delaying the stall (and making it worse, since ForgetSkippable is faster than the CC without it)? Or is the whole CC skippable in this shutdown case? I know there are situations where we need to finalize (or discard, or whatever it's called in the CC world) cyclic garbage objects, but I thought we also terminated content process abruptly already too.

mccr8, maybe?

Comment 11

[Andrew McCreight [:mccr8]]

We don't do shutdown CCs in non-leak checking builds.

Comment 12

[Jens Stutte [:jstutte]]

I moved the GC part to bug 1790360.