profiler icon indicating copy to clipboard operation
profiler copied to clipboard

[Deploy preview] Allow consecutive samples with the same timestamp

Open mstange opened this issue 3 years ago • 2 comments

Production (empty activity graph) Deploy preview (normal activity graph)

My generator sometimes outputs two consecutive samples with the same timestamp. This happens if there is an off-cpu-sample for the entire "thread sleep duration" immediately followed by an on-cpu-sample with the wake up timestamp. These samples were breaking activity graph drawing.

The time delta is zero, so we were dividing by zero and creating NaN values. And then the smoothing blurred the NaN over the entire length of the canvas. And then we couldn't draw a graph because everything was NaN.

mstange avatar Jun 03 '22 16:06 mstange

Ah, thanks for the fix! I didn't think about having zero time difference I guess when I was working on it. Place of this calculation changed though after PR #4067. Sorry about the conflict. Do you intend to land it soon (since it's work in progress right now)?

canova avatar Jun 08 '22 11:06 canova

This isn't blocking me at the moment so I'm probably not going to work on it. Feel free to take it over. Oh, and it probably should have a test...

It's not blocking me because I decided to change my generator so that it only produces off-cpu samples at a certain sample rate, rather than one sample for every context switch. That's because the profiles ended up being big and slow due to the large number of samples. Context switches occur really frequently. And then I probably won't have samples with the same timestamp anymore.

mstange avatar Jun 08 '22 14:06 mstange

I think this is fixed now. I can't find any code that divides by intervalDistribution any more.

mstange avatar Dec 08 '22 18:12 mstange

I think this is fixed now. I can't find any code that divides by intervalDistribution any more.

It's not fixed - the link in https://github.com/firefox-devtools/profiler/pull/4068#issue-1260079224 still doesn't work.

There's a spot where we divide by sampleDeltaInMs: https://github.com/firefox-devtools/profiler/blob/358a1c77111edf944d09fbe79012a08832a9cb59/src/profile-logic/cpu.js#L41

mstange avatar Dec 03 '23 21:12 mstange

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (1b08327) 88.30% compared to head (0710057) 88.30%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4068   +/-   ##
=======================================
  Coverage   88.30%   88.30%           
=======================================
  Files         301      301           
  Lines       26968    26969    +1     
  Branches     7290     7291    +1     
=======================================
+ Hits        23814    23815    +1     
  Misses       2936     2936           
  Partials      218      218           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 03 '23 21:12 codecov[bot]