Avoid saving every checkpoint when rewinding or warping

RESOLVED FIXED in Firefox 67

Status

()

enhancement
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

5 months ago
Posted patch patchSplinter Review

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.

Assignee

Comment 1

5 months ago

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.

Attachment #9040294 - Flags: review?(continuation)
Assignee

Comment 2

5 months ago

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.

Attachment #9040295 - Flags: review?(lsmyth)
Attachment #9040294 - Flags: review?(continuation) → review+
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 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?
Assignee

Comment 5

5 months ago

(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.

Assignee

Comment 6

5 months ago

(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.

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.

Assignee

Comment 8

5 months ago

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.

Attachment #9040295 - Attachment is obsolete: true
Attachment #9040295 - Flags: review?(lsmyth)
Attachment #9040942 - Flags: review?(lsmyth)
Attachment #9040942 - Flags: review?(lsmyth) → review+

Comment 9

4 months ago
Pushed by bhackett@mozilla.com:
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

4 months ago
bugherder
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.