1524127 - Avoid saving every checkpoint when rewinding or warping

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Right now, when rewinding we require the rewinding process to have saved every checkpoint going back to its last major checkpoint. As the replaying processes alternate back and forth, we end up having to save every checkpoint over the range being rewound past. When checkpoints are very closely spaced this is pretty inefficient, and it would be nice to relax this constraint. The attached patch does this by having an concept of minor checkpoints analogous to major checkpoints. The major checkpoints are saved as we run forward, and the minor checkpoints are saved as we run backwards. The minor checkpoints are more closely spaced than the major checkpoints (.25 seconds vs. 2 seconds) but this is easily adjustable and will make things respond better if we start adding more checkpoint locations in the future.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Support searching regions spanning multiple checkpoints.

Right now the navigation phases only support running through or searching parts of the execution space that are between adjacent checkpoints. In order to avoid having to save every checkpoint when rewinding, this needs to be relaxed. The attached patch makes this fix, which includes a related fix to make sure we can still paint graphics when rewinding. When rewinding we will only show paints that are associated with minor checkpoints, so graphics will be choppy, but compared with the speed improvements this is a good trade/off to make.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Keep track of minor checkpoints in control logic.

Keep track of minor checkpoints along with major checkpoints in the record/replay control logic. Only minor checkpoints need to be saved when rewinding, and intermediate checkpoints don't need to be saved at all when warping.

Comment 3

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9040294 [details] [diff] [review]
Part 1 - Support searching regions spanning multiple checkpoints.

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

::: toolkit/recordreplay/ProcessRewind.cpp
@@ +250,5 @@
> +GetLastSavedCheckpointPriorTo(const CheckpointId& aCheckpoint)
> +{
> +  MOZ_RELEASE_ASSERT(HasSavedCheckpoint());
> +  for (size_t i = gRewindInfo->mSavedCheckpoints.length(); i >= 1; i--) {
> +    if (gRewindInfo->mSavedCheckpoints[i].mCheckpoint == aCheckpoint) {

Won't this be an out-of-bounds index for the initial `length` access?

Comment 4

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9040295 [details] [diff] [review]
Part 2 - Keep track of minor checkpoints in control logic.

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

::: devtools/server/actors/replay/control.js
@@ +154,5 @@
>      return this._majorCheckpoints.some(major => major == id);
>    },
>  
> +  isMinorCheckpoint(id) {
> +    return this._minorCheckpoints.some(minor => minor == id);

This seems like a good usecase for a Set. What's the motivation for using an array?

@@ +184,4 @@
>             this._willSaveCheckpoints.includes(id);
>    },
>  
> +  hasSavedMinorCheckpointsPreceding(id) {

Looks like this will return `true` if there are no minor checkpoints at all? Is the assumption that there will always be at least one?

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Logan Smyth [:loganfsmyth] from comment #3)

Comment on attachment 9040294 [details] [diff] [review]
Part 1 - Support searching regions spanning multiple checkpoints.

Review of attachment 9040294 [details] [diff] [review]:

::: toolkit/recordreplay/ProcessRewind.cpp
@@ +250,5 @@

+GetLastSavedCheckpointPriorTo(const CheckpointId& aCheckpoint)
+{

• MOZ_RELEASE_ASSERT(HasSavedCheckpoint());
• for (size_t i = gRewindInfo->mSavedCheckpoints.length(); i >= 1; i--) {
• if (gRewindInfo->mSavedCheckpoints[i].mCheckpoint == aCheckpoint) {

Won't this be an out-of-bounds index for the initial length access?

Oops, good catch.

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Logan Smyth [:loganfsmyth] from comment #4)

Comment on attachment 9040295 [details] [diff] [review]
Part 2 - Keep track of minor checkpoints in control logic.

Review of attachment 9040295 [details] [diff] [review]:

::: devtools/server/actors/replay/control.js
@@ +154,5 @@

 return this._majorCheckpoints.some(major => major == id);

},

• isMinorCheckpoint(id) {
• return this._minorCheckpoints.some(minor => minor == id);

This seems like a good usecase for a Set. What's the motivation for using an
array?

Well, the array is there because it is consistent with what we do for _majorCheckpoints. The code in this file generally doesn't use very efficient data structures and I'm planning on doing a pass at some point to fix that, but I can take care of that now for _minorCheckpoints.

@@ +184,4 @@

        this._willSaveCheckpoints.includes(id);

},

• hasSavedMinorCheckpointsPreceding(id) {

Looks like this will return true if there are no minor checkpoints at all?
Is the assumption that there will always be at least one?

Returning true if there are no minor checkpoints in the range is the correct behavior. After all, in such situations they all will have been saved.

Comment 7

[Logan Smyth [:loganfsmyth]]

The code in this file generally doesn't use very efficient data structures and I'm planning on doing a pass at some point to fix that

It's totally a nitpick, don't worry about it.

Returning true if there are no minor checkpoints in the range is the correct behavior. After all, in such situations they all will have been saved.

That's totally fine, but it seems to unexpected given the name of the function, since there are no saved minor checkpoints at preceding, but it can still return true.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Keep track of minor checkpoints in control logic.

This revised patch uses a Set for minor checkpoints and renames hasSavedMinorCheckpointsPreceding(id) to canRewindFrom(id) to better fit with the meaning of the checks being performed here.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51e195638ef6
Part 1 - Support searching regions spanning multiple checkpoints, r=mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e98200ee530
Part 2 - Keep track of minor checkpoints in control logic, r=lsmyth.

Comment 10

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/51e195638ef6
https://hg.mozilla.org/mozilla-central/rev/4e98200ee530