profiler icon indicating copy to clipboard operation
profiler copied to clipboard

Clicking instant markers in the timeline causes a preview selection with a strange range

Open mstange opened this issue 3 years ago • 3 comments

Profile: https://share.firefox.dev/3NdoSjR

Steps to reproduce:

  1. In the Renderer thread timeline, find any "Composite" marker, and move your mouse slightly to the right so that you're over a "Render reason VSYNC" marker.
  2. Click the "Render reason VSYNC" marker.

Expected results: "Render reason VSYNC" is an instant marker, so I'd expect nothing to happen. Actually I wouldn't expect to see these types of markers in the timeline at all.

Actual results: The preview selection is changed to a 159ms range centered around the clicked marker.

This was very confusing to me because I had intended to click a "Composite" marker, so I was expecting to get a selection that spans one composite. But the selection I ended up with was much longer than a composite and it made me wonder "What are we doing that's taking 159ms?"

mstange avatar May 15 '22 14:05 mstange

Hello. I want to work on this issue

barisconur avatar May 18 '22 02:05 barisconur

What I figured out so far: The length of preview selection is calculated in Selection.js with the formula below. const selectionWidth = ((selectionEnd - selectionStart) / (committedRange.end - committedRange.start)) * width;

The value of committedRange parameters and width is same for all markers. When I click on any marker, it takes selectionEnd and selectionStart parameter values as a prop. I logged the values for the composite marker and VSYNCH marker which is closer to the composite marker. It returns faulty values for VSYNCH marker.

I am not familiar with the codebase so I could not find where we are updating the prop values. But hierarchy is like: image

barisconur avatar May 18 '22 02:05 barisconur

Actually I wouldn't expect to see these types of markers in the timeline at all.

My understanding is that they use the TimelineOverview position because they use the Tracing schema. See https://searchfox.org/mozilla-central/rev/9f95c41a962c9228f569f8a6b2c30edbb50b65ae/gfx/webrender_bindings/src/bindings.rs#887 Maybe instead they should use a TextMarker through https://searchfox.org/mozilla-central/rev/9f95c41a962c9228f569f8a6b2c30edbb50b65ae/gfx/webrender_bindings/src/bindings.rs#891 Or come up with new types. But I don't know enough about the Rust API to give better information. What do you think?

The preview selection is changed to a 159ms range centered around the clicked marker.

This comes from this code: https://github.com/firefox-devtools/profiler/blob/630c48babd770aa2a3398661b0b0c1b90ced525a/src/utils/index.js#L74-L94

I still feel like it's useful generally and that the problem here is that those markers shouldn't be in the timeline.

What do you think?

Hello. I want to work on this issue

Hey @barisconur, we're not sure what to do here yet, so this issue isn't ready to be worked on :-) thanks for your understanding

julienw avatar May 20 '22 16:05 julienw