Screenshots track

[gregtatum]

I'm throwing this up early as the UI comes together for feedback.

Update: This is now ready for review. It is built on top of the network track work, so please disregard those commits. They are labeled (do not review).

Resolves #1056

Deploy preview #1

Deploy preview (window resize)

Deploy preview cnn.com load/scroll

[codecov]

Codecov Report

Merging #1198 into master will increase coverage by 0.43%.
The diff coverage is 98.4%.

@@            Coverage Diff             @@
##           master    #1198      +/-   ##
==========================================
+ Coverage   76.06%   76.49%   +0.43%     
==========================================
  Files         146      147       +1     
  Lines        9501     9612     +111     
  Branches     2349     2365      +16     
==========================================
+ Hits         7227     7353     +126     
+ Misses       2031     2017      -14     
+ Partials      243      242       -1

Impacted Files	Coverage Δ
src/components/timeline/GlobalTrack.js	87.32% <100%> (+7.32%)	⬆️
src/profile-logic/marker-data.js	74.82% <100%> (+3.96%)	⬆️
src/test/fixtures/profiles/make-profile.js	97.28% <100%> (+0.63%)	⬆️
src/profile-logic/tracks.js	95.28% <100%> (+1.19%)	⬆️
src/reducers/profile-view.js	94.8% <100%> (+0.1%)	⬆️
src/components/timeline/TrackScreenshots.js	97.01% <97.01%> (ø)
src/components/timeline/TracingMarkers.js	82.22% <0%> (+1.48%)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14e3842...13d9dd0. Read the comment docs.

[julienw]

This is a quite solid work, I really like how this works and behaves.

My main comments are:

• the fact that the data is stored as markers makes it a bit uneasy to work with. Maybe this should be extracted at loading and processing time, and put in a separate structure in the processed format ?
• we miss screenshots when the profile is zoomed in (I left a more detailed comment in the reducers file and some suggestions in the component file)
• the algorithm to find the right screenshot to display at hover seems to have a off by one error (more info in the component file)

Because of these issues I can't approve yet. I believe landing this like it is would be technical debt to fix later, but in the same time having the screenshots working, even imperfectly, is a huge addition. So I'm happy to discuss if you prefer to defer these changes.

Otherwise nits and comment requests.

[julienw]

can I haz exact and stricter type ?

[julienw]

optional: Another option that avoids twisting Flow's figurative arm:

if (typeof data.windowID === "string") {
  ids.add(data.windowID);
}

[gregtatum]

This still gives me Flow errors.

[julienw]

I really think we need a value for type here; there are a lot of places we use this property and not having one is not a good idea. I think we have a bugzilla issue to fix the missing type for the payload for VsyncTimestamp, and we need to do the same for this payload.

[gregtatum]

I believe this will be resolved with the proposed marker processing step discussed on Slack.

[julienw]

I'm a bit concerned about the hardcoded name 'CompositorScreenshot' here, and I wonder if the screenshots shouldn't be in a separate structure than markers. I understand why it's easier for the gecko profile format, but maybe that's something that could be preprocessed at loading time and put in a separate structure in the processed format ?
That would make everything simpler, including flow typing...

[gregtatum]

I have a pretty big preference to keeping with the MarkerTable practice, rather than a secondary data structure, especially given the follow-up work with the MarkerChart processing.

As for the string, I'm not sure on the concern for having a hard coded string, as we key off of many values from the marker table using a string.

[julienw]

nit: leftover ?

[gregtatum]

But but... it does so many things.

[julienw]

maybe add that it makes it easier to define payloads where we're just interested in this data ?

but maybe it's just as easy to literally add the dummy type to these tests that use this "feature", and make the logic here dumber... (optional for this PR)

[gregtatum]

I added the type everywhere originally, but it made the tests a lot uglier, so I prefer this approach.

[julienw]

if you follow my suggestion of not range filtering the screenshot data (see my comment in the reducers file), then you should likely start at 0 here.

[julienw]

should we compare to left + imageContainerWidth / 2, so that a screenshot's center (instead of its left) would match the screenshot's time ?

[gregtatum]

I would say we should base it on time, not based on the screenshot width, that way the left-hand side represents the current time. I believe this is how film editing programs do it.

[julienw]

okay, I'm not familiar with this so let's do like you say :)

[julienw]

if you follow my suggestion of having all screenshots in the received prop (see my comment in the reducers file), we'll need to find where to start displaying a screenshot. So I think it should be set to null until we find a proper index in the following loop. Then if it's null after the "screenshot finding" for loop we call continue.

Or we can try to find the first one to display here, maybe with bisect.

[julienw]

There's one problem in using getRangeFilteredThread here: we don't get the screenshot that's immediately before this range.

This is a problem for deeply zoomed-in views, eg this view: we miss the first part, where we should see the screenshot immediately before.

This means that maybe we should always return all screenshots... the good thing is that the structure would be calculated only once, so maybe it could be a direct win ?

[gregtatum]

This will be addressed in the MarkerTable processing step follow-up.

[julienw]

I still think we need to address this issue now.

Looks like I forgot to add the link that exhibits the issue in my previous comment, so here are some links exhibiting the user-facing issues coming from this bug. These links are found from your deploy previews, and then zooming in a lot.

1. a small range will yield a blank space at the start despite the fact that there are screenshots available before this time.

I don't completely get why the MarkerTable processing will change this. Maybe we don't understand each other about the issue I saw here, and I hope the link above will help. Happy to discuss :)

While testing this I also found the next slightly related problem, with 2 examples:

• a smaller range will yield an Error and a blank page
• we get the exact same error when simply selecting a range in a location where there are no screenshot (like at the start of a profile).

These 2 last errors come from https://github.com/devtools-html/perf.html/pull/1198/files#diff-ecb2e49c58365594ed52995be3de2933R240: we have no data for this id because there's no screenshot data in this range. I believe we should fix this anyway because even with the fix for 1 this may happen and we should never hard throw like this. I outlined an easy solution there.

[gregtatum]

@julienw This should be ready for re-review.

[julienw]

I still think we can't land this with the issue that we miss some screenshots when zooming in even when we have them. I left more information in comments but I'm happy to discuss. If you feel strongly about keeping it like this, then I'll approve later, because I think it's better to land it like this than not having it at all, but I'd be a bit sad.

I also found another issue while testing this, that we badly crash in some cases, but that will be easy to fix.

[julienw]

actually, maybe we'll want to use bisect if we find that this is too costly to loop like this in some busy profiles on each mouseover. But let's do it later if we need it.

[julienw]

I still think we need to address this issue now.

Looks like I forgot to add the link that exhibits the issue in my previous comment, so here are some links exhibiting the user-facing issues coming from this bug. These links are found from your deploy previews, and then zooming in a lot.

1. a small range will yield a blank space at the start despite the fact that there are screenshots available before this time.

I don't completely get why the MarkerTable processing will change this. Maybe we don't understand each other about the issue I saw here, and I hope the link above will help. Happy to discuss :)

While testing this I also found the next slightly related problem, with 2 examples:

• a smaller range will yield an Error and a blank page
• we get the exact same error when simply selecting a range in a location where there are no screenshot (like at the start of a profile).

These 2 last errors come from https://github.com/devtools-html/perf.html/pull/1198/files#diff-ecb2e49c58365594ed52995be3de2933R240: we have no data for this id because there's no screenshot data in this range. I believe we should fix this anyway because even with the fix for 1 this may happen and we should never hard throw like this. I outlined an easy solution there.

[julienw]

This throws if we have no screenshot data for this id. This can happen more often than we think (eg: zooming in to a range that has no screenshot data) so I think we should handle this better.

Maybe the easiest is to return an empty array here, because the component code handles the empty array case properly:

      screenshots: selectors.getScreenshotMarkersById(state).get(screenshotId) || EMPTY_ARRAY,

(of course EMPTY_ARRAY needs to be a constant for shallow equality checks).

Please also add a test for this case.

[gregtatum]

I changed this to have a selector that always returns an empty TracingMarker[] when zooming in on a track.

[gregtatum]

The follow-up markers patch gets rid of tracing markers, and correctly filters markers by the range, so calling getRangeFilteredMarkers gets the markers correctly filtered.

export function filterMarkersToRange<Payload>(
  markers: MarkersTableByType<Payload>,
  rangeStart: Milliseconds,
  rangeEnd: Milliseconds
): MarkersTableByType<Payload> {
  const newMarkers: MarkersTableByType<*> = {
    startTime: [],
    endTime: [],
    duration: [],
    type: [],
    name: [],
    title: [],
    data: [],
    length: 0,
  };

  for (let markerIndex = 0; markerIndex < markers.length; markerIndex++) {
    const startTime = markers.startTime[markerIndex];
    let endTime = markers.endTime[markerIndex];
    if (endTime === null) {
      endTime = startTime;
    }

    if (startTime < rangeEnd && endTime >= rangeStart) {
      newMarkers.startTime.push(markers.startTime[markerIndex]);
      newMarkers.endTime.push(markers.endTime[markerIndex]);
      newMarkers.duration.push(markers.duration[markerIndex]);
      newMarkers.type.push(markers.type[markerIndex]);
      newMarkers.name.push(markers.name[markerIndex]);
      newMarkers.title.push(markers.title[markerIndex]);
      newMarkers.data.push(markers.data[markerIndex]);
      newMarkers.length++;
    }
  }
  return newMarkers;
}

    const getRangeFilteredMarkers = createSelector(
      getMarkers,
      getCommittedRange,
      (markers, { start, end }) =>
        MarkerData.filterMarkersToRange(markers, start, end)
    );

[gregtatum]

@julienw I fixed the range issues you pointed out by converting the markers into TracingMarkers, as TracingMarkers already have tools in place to deal with these range selection issues. This is ready for re-review.

(edited for tone, please see follow-up comment)

[gregtatum]

Please see #1227 for some work on TracingMarkers.

My preference here is strongly towards using common infrastructure such as TracingMarkers, and working towards a common set of selectors and types that can be consistent across our marker usage. I think TracingMarkers and their future evolutions are the way to go here. #1227 has more justifications, but I can go into more detail.

My pushback here is not around having to change behavior of existing component code, but on trying to find a path towards fewer ad hoc rules around markers, and a more consistent processed marker view that makes it much easier to work with the marker data.

[julienw]

Thanks for the rich and clear explanations of your approach. I wouldn't have taken the same path, but it doesn't mean my approach is better. Having a more consistent way of dealing with markers is definitely beneficial.

I couldn't find any more issue so I'm gonna approve this patch. I left a few nits and requests still, some of them are optional though.

Thanks, I can't wait having this in production !

[julienw]

I think that the fact that it won't be null is now OK. But this relies on a lot of somewhat implicit assumptions in 1. how we compute the global tracks (the rule that there won't be a screenshot track if there are no screenshot markers), and 2. how we compute this selector (the rule that if there's at least a screenshot marker for this id in the thread, there will always be a non-null result).

Therefore, if you want to keep this ensureExists here, I'd like some solid tests to make sure we don't regress in the future.
For 2. you wrote a test already so it's good (the test where we reduce the range).
For 1. I'd like that you check if such a test exists: merely a full snapshot test for a profile that doesn't have any screenshot markers. Maybe we have one already. Maybe it makes sense to explicitely add one with the proper title even if we implicitely test it already.

Alternatively, you can make sure to pass an empty array as screenshots if the map doesn't have this ID.