New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Screenshots track #1198

Merged
merged 9 commits into from Aug 30, 2018

Conversation

Projects
None yet
2 participants
@gregtatum
Member

gregtatum commented Aug 15, 2018

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

This comment has been minimized.

codecov bot commented Aug 15, 2018

Codecov Report

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

Impacted file tree graph

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

@gregtatum gregtatum force-pushed the gregtatum:screenshot-track branch 4 times, most recently from 8805da5 to 759d38d Aug 16, 2018

@gregtatum gregtatum requested a review from julienw Aug 21, 2018

@gregtatum gregtatum changed the title from [WIP] Screenshots track to Screenshots track Aug 21, 2018

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

// stored in the string table will be scaled down from the original size.
windowWidth: number,
windowHeight: number,
};

This comment has been minimized.

@julienw

julienw Aug 23, 2018

Collaborator

can I haz exact and stricter type ?

// Coerce the payload to a screenshot one. Don't do a runtime check that
// this is correct.
const data: ScreenshotPayload = (markers.data[markerIndex]: any);
ids.add(data.windowID);

This comment has been minimized.

@julienw

julienw Aug 23, 2018

Collaborator

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

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

This comment has been minimized.

@gregtatum

gregtatum Aug 27, 2018

Member

This still gives me Flow errors.

@@ -346,6 +346,20 @@ type VsyncTimestampPayload = {|
vsync: 0,
|};

export type ScreenshotPayload = {
// The "type" property doesn't exist, but is required to make Flow typing work.
type: void,

This comment has been minimized.

@julienw

julienw Aug 23, 2018

Collaborator

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.

This comment has been minimized.

@gregtatum

gregtatum Aug 27, 2018

Member

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

if (stringTable.hasString('CompositorScreenshot')) {
const screenshotNameIndex = stringTable.indexForString(
'CompositorScreenshot'
);

This comment has been minimized.

@julienw

julienw Aug 23, 2018

Collaborator

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

This comment has been minimized.

@gregtatum

gregtatum Aug 27, 2018

Member

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.

right: 0;
}
.timelineTrackScreenshotImg {
}

This comment has been minimized.

@julienw

julienw Aug 23, 2018

Collaborator

nit: leftover ?

This comment has been minimized.

@gregtatum

gregtatum Aug 27, 2018

Member

But but... it does so many things.

/**
* This function ensures that the mock payloads are converted correctly to real payloads
* that match the MarkerPayload typing. Specifically it adds the 'DummyForTests' type
* to { startTime, endTime } payloads.

This comment has been minimized.

@julienw

julienw Aug 23, 2018

Collaborator

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)

This comment has been minimized.

@gregtatum

gregtatum Aug 27, 2018

Member

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


let screenshotIndex = 0;
for (
let left = timeToPixel(screenshots.time[0]);

This comment has been minimized.

@julienw

julienw Aug 23, 2018

Collaborator

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.

) {
// Try to find the next screenshot to fit in, or re-use the existing one.
for (let i = screenshotIndex; i < screenshots.length; i++) {
if (timeToPixel(screenshots.time[i]) <= left) {

This comment has been minimized.

@julienw

julienw Aug 23, 2018

Collaborator

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

This comment has been minimized.

@gregtatum

gregtatum Aug 27, 2018

Member

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.

This comment has been minimized.

@julienw

julienw Aug 28, 2018

Collaborator

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

const timeToPixel = time =>
outerContainerWidth * (time - rangeStart) / rangeLength;

let screenshotIndex = 0;

This comment was marked as outdated.

@julienw

julienw Aug 23, 2018

Collaborator

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.

MarkerTiming.getMarkerTiming
);
const getScreenshotMarkersById = createSelector(
getRangeFilteredThread,

This comment has been minimized.

@julienw

julienw Aug 23, 2018

Collaborator

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 ?

This comment has been minimized.

@gregtatum

gregtatum Aug 27, 2018

Member

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

This comment has been minimized.

@julienw

julienw Aug 28, 2018

Collaborator

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:

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 gregtatum force-pushed the gregtatum:screenshot-track branch from 4f753bf to cdb228b Aug 27, 2018

@gregtatum

This comment has been minimized.

Member

gregtatum commented Aug 27, 2018

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

if (time >= mouseTime) {

// Loop backwards to find the latest screenshot that has a time less
// than the current time at the mouse position.

This comment has been minimized.

@julienw

julienw Aug 28, 2018

Collaborator

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.

MarkerTiming.getMarkerTiming
);
const getScreenshotMarkersById = createSelector(
getRangeFilteredThread,

This comment has been minimized.

@julienw

julienw Aug 28, 2018

Collaborator

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:

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.

screenshots: ensureExists(
selectors.getScreenshotMarkersById(state).get(screenshotId),
'Expected to find screenshots for the given pid'
),

This comment has been minimized.

@julienw

julienw Aug 28, 2018

Collaborator

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.

This comment has been minimized.

@gregtatum

gregtatum Aug 29, 2018

Member

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

@gregtatum

This comment has been minimized.

Member

gregtatum commented Aug 29, 2018

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 gregtatum force-pushed the gregtatum:screenshot-track branch from cdb228b to 52c4803 Aug 29, 2018

@gregtatum

This comment has been minimized.

Member

gregtatum commented Aug 29, 2018

@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

This comment has been minimized.

Member

gregtatum commented Aug 29, 2018

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 !

Show resolved Hide resolved src/test/components/TrackScreenshots.test.js Outdated
Show resolved Hide resolved src/reducers/profile-view.js Outdated
Show resolved Hide resolved src/reducers/profile-view.js Outdated
Show resolved Hide resolved src/components/timeline/TrackScreenshots.js Outdated
Show resolved Hide resolved src/components/timeline/TrackScreenshots.js Outdated
Show resolved Hide resolved src/test/store/profile-view.test.js
Show resolved Hide resolved src/test/store/profile-view.test.js
Show resolved Hide resolved src/test/store/profile-view.test.js Outdated
return {
thread: selectors.getRangeFilteredThread(state),
screenshots: ensureExists(
selectors.getRangeFilteredScreenshotsById(state).get(screenshotId),

This comment has been minimized.

@julienw

julienw Aug 30, 2018

Collaborator

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.

Show resolved Hide resolved src/components/timeline/TrackScreenshots.js Outdated

@gregtatum gregtatum force-pushed the gregtatum:screenshot-track branch from 52c4803 to 13d9dd0 Aug 30, 2018

@gregtatum gregtatum merged commit 64fcd06 into devtools-html:master Aug 30, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 98.4% of diff hit (target 76.06%)
Details
codecov/project 76.49% (+0.43%) compared to 14e3842
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment