search

catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.93k stars 562 forks source link

Tracks should be pinnable #1790

Open zeptonaut opened 8 years ago

zeptonaut commented 8 years ago

Tracks in trace viewer should be "pinnable", in a similar way that Chrome tabs are pinnable:

zeptonaut commented 8 years ago

One interesting problem: if we want to be able to pin tracks, then we presumably also want to be able to unpin tracks. When we unpin one, we presumably want it to go back to where it was in the ordering before being pinned. But how do we know where it was in the ordering?

A couple of ideas:

  1. We could store its parent. However, this isn't sufficient to identify where to place the node among its siblings.
  2. We could store its parent as well as its predecessor sibling. However, if the predecessor sibling also gets pinned afterward, then when we unpin the initially pinned track, it'll be placed as the next node after the stored sibling - still among the pinned tracks. Instead, it really should go as the next node after it's predecessor's predecessor... unless that was pinned as well.
  3. We could do the above, but every time that a track gets pinned, we see if any pinned tracks hold a reference to that track as the predecessor. If they do, we set those tracks to reference the newly pinned tracked's predecessor instead. This would require some bookkeeping, but might not be too bad.
  4. We could fake pinning tracks. Every time that a track gets pinned, we create a copy of that track among the pinned tracks and set the original track's visibility to hidden. In the copy, we then store a link to the original track. If the track gets unpinned, we show the original track (which we have a link to) and delete the pinned copy of the track. I have three primary concerns with this:
    1. Some tracks have lots of DOM elements, and I'm concerned that copying the track is going to take time
    2. Some tracks have lots of DOM elements, and copying the track means that we're now taking up twice as much memory for that track
    3. We don't have any way to do a deep copy of a track at the moment. I'm not sure if this is necessary, but it seems like it might be
zeptonaut commented 8 years ago

Alright - after thinking a little bit more about this, I think that option 3 might be our best bet.

One concern that I had was with track nesting. Suppose that we had 3 tracks, nested like so:

Suppose that we start out by pinning track a. The tracks now look like this:

Pinned tracks

Normal tracks

Now suppose that we pin track 1. The end result that I think we should see is this:

Pinned tracks

Normal tracks

(empty)

In order to do this, when we go to pin track 1, we need to check if any of the already-pinned tracks have it as an ancestor. We do this by checking the originalParent field of the pinned track, then following the DOM up, looking for the track currently being pinned. If we don't find it, great: just go ahead and pin the new track. If we do find it, that's okay too: we just have to unpin the existing track (moving it back to its original location in the DOM), then pin the new track, and everything should look like we want it to above.

This bookkeeping is definitely a little annoying, but I'm not sure I see any obvious ways around it. I also think that we should be able to keep all of it within some sort of a PinnableTrack base class. This base class is necessary, anyhow, because not all tracks should be pinnable (pinning a single rect-track within an async-slice track would be silly).

natduca commented 8 years ago

Would it be useful to separate out the class hierarchy that creates the tracks, from the layout of the tracks? I think the 1:1 relationship between tracks and the containers is really problematic for future ui that we want anyway...

Basically I'm wondering if this is one of those cases where "the simple feature ends up being jut as hard as doing the right thing, so lets figure out the right thing instead."

On Tue, Mar 8, 2016 at 7:05 AM, Charlie Andrews notifications@github.com wrote:

Alright - after thinking a little bit more about this, I think that option 3 might be our best bet.

One concern that I had was with track nesting. Suppose that we had 3 tracks, nested like so:

  1. Track 1
    1. Track i
      1. Track a

Suppose that we start out by pinning track a. The tracks now look like this:

Pinned tracks

  1. Track a (With 'originalParent' set to 'Track i')

Normal tracks

  1. Track 1
    1. Track i

Now suppose that we pin track 1. The end result that I think we should see is this:

Pinned tracks

  1. Track 1
    1. Track i
      1. Track a

Normal tracks

(empty)

In order to do this, when we go to pin track 1, we need to check if any of the already-pinned tracks have it as an ancestor. We do this by checking the originalParent field of the pinned track, then following the DOM up, looking for the track currently being pinned. If we don't find it, great: just go ahead and pin the new track. If we do find it, that's okay too: we just have to unpin the existing track (moving it back to its original location in the DOM), then pin the new track, and everything should look like we want it to above.

This bookkeeping is definitely a little annoying, but I'm not sure I see any obvious ways around it.

— Reply to this email directly or view it on GitHub https://github.com/catapult-project/catapult/issues/1790#issuecomment-193816351 .

zeptonaut commented 8 years ago

I'm not sure I understand. I totally get the separation of the view and model . But aren't tracks a view-only concept? Shouldn't the only representation of them be in the view?

natduca commented 8 years ago

Right now tracks are 1:1 to DOM elements and to model objects, so the DOM embodiment, layotu etc are all entangled.

I was muttering about maybe we should have a thing that walks the model to generate a list of actual renderable tracks and some potential grouping, e.g. rect_tracks tagged with a process name as a group. Etc.

Then we have another system that consumes that list, maybe shufffles it around if people have dragged or pinned, then creates the DOM entities that lay the actual track tree out.

On Tue, Mar 8, 2016 at 10:10 AM, Charlie Andrews notifications@github.com wrote:

I'm not sure I understand. I totally get the separation of the view and model . But aren't tracks a view-only concept? Shouldn't the only representation of them be in the view?

— Reply to this email directly or view it on GitHub https://github.com/catapult-project/catapult/issues/1790#issuecomment-193896495 .

zeptonaut commented 8 years ago

Looking through the code, I'm not super concerned about the entanglement between the tracks and the DOM elements being a major blocker to the implementation of any track pinning logic. The bigger blockers that I've noticed are more around cleanliness of the track view code. For example, a logical place to put some sort of a "pin this track" icon would be in the track heading next to the title of the track. I think it's a decently safe assumption that any track that's important enough to have a heading should be pinnable. However, it turns out that every track has a heading, but it's just empty for a lot of tracks. We do this because we use the heading as a means of making the track title area blue with a black right border.

I think that there were some other strange structural problems like this that I've run into while trying to pin tracks as well. IMO, when implementing track pinning, we'd reap more rewards from fixing these track-view annoyances than taking a more major detour to change the track model altogether.