1505229 - Never wait for replaying child processes while recording

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Middleman processes often block the main thread while waiting for a child process to reach a certain point in execution.  Many of these are necessary, owing to the synchronous nature of operations like time warps or debugger requests.  Some of these waits are unnecessary, though.  In particular, when the tab is recording new data there is no reason we should ever have to block while waiting for one of the replaying children to do something; the replaying children are in a standby role and their sole purpose is to run forward and catch up with the recording child's position.

We pay a huge performance penalty for this waiting.  On a sample run of loading GMail, we spend a total of nearly 9 seconds (!) blocked here, with the recording child usually paused as well.

The attached patch fixes the two reasons why we might wait in this way:

- Currently, when flushing the recording we wait for all replaying children to pause before instructing the recording child to write the new data to disk.  With this patch, we allow recording to continue and the replaying children to pause of their own accord (and stay paused) before instructing the recording child to write out the new data.

- Currently, when assigning a major checkpoint to a replaying child (a checkpoint which will always be saved) we wait for the child to pause before telling it to save the checkpoint.  With this patch, we don't do this until the child has paused at the checkpoint immediately before the major checkpoint; this dovetails well with existing code that clears out non-major checkpoints the child has previously been instructed to save.

This results in a big performance improvement when loading complex sites.

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 9023142 [details] [diff] [review]
patch

Review of attachment 9023142 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/recordreplay/ipc/ParentIPC.cpp
@@ +383,1 @@
>      aChild->SendMessage(SetSaveCheckpointMessage(aCheckpoint, false));

The logic in this method seems a little convoluted. Maybe you could do something like this:

bool childShouldSave = aChild->IsMajorCheckpoint(aCheckpoint) || aCheckpoint == CheckpointId::First;
bool childToldToSave = aChild->ShouldSaveCheckpoint(aCheckpoint);

if (childShouldSave != childToldToSave) {
  aChild->SendMessage(SetSaveCheckpointMessage(aCheckpoint, childShouldSave));
}

At least to me, that makes it a little clearer what is going on.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4858cc391c4d
Never wait for replaying child processes while recording, r=mccr8.

Comment 3

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4858cc391c4d

Comment 4

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4562