Simplify some code related to thread CPU deltas #5265
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5265 +/- ##
==========================================
- Coverage 88.62% 88.61% -0.01%
==========================================
Files 308 308
Lines 28105 28095 -10
Branches 7621 7618 -3
==========================================
- Hits 24908 24897 -11
- Misses 2983 2984 +1
Partials 214 214 ☔ View full report in Codecov by Sentry. |
mstange
force-pushed
the
push-puyntwqusqyq
branch
2 times, most recently
from
e9e439d to
9a5f97b
Compare
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
This looks reasonable to me, but I'd very much like to have feedback from Nazım, because he knows a lot more about the CPU values than I do. Unfortunately he's now away until next year, I hope that's OK.
| expect(processedThread1.samples.threadCPUDelta).toEqual([ | ||
| 0.1, 0.1, 0.1, 0.2, 0.2, 0.2, | ||
| 0.1, 0, 0, 0, 0, 0.2, | ||
| ]); |
There was a problem hiding this comment.
From my understanding:
- before we were treating "null" as "I don't know", and as a result we were, in a way, splitting the "I don't know" hole in half, with the first half using the previous value, and the second half using the next value.
- With your change, the "I don't know part" is transformed into a "0".
I don't know if that's OK. Especially I don't know how common it is to have null values and what that means. I'd like to have Nazim feedback on this, if that's OK?
There was a problem hiding this comment.
After checking 1000 profiles, I have found the following:
- There can be more than one
nullvalue at the start of the array. - In every checked profile, once you see a non-
nullvalue, the rest of the values are also non-null.
And the reason for multiple null values at the beginning appears to be that we never implemented threadCPUDelta in the base profiler! So during process startup, you get all nulls until the non-base profiler takes over. Example: https://share.firefox.dev/4iG7vsj
There was a problem hiding this comment.
There was a problem hiding this comment.
Given Florian's confusion in bug 1756519, I would say zeroing out the base profiler values is preferable over what we do today: It'll be more obviously-wrong and less plausible-but-wrong.
There was a problem hiding this comment.
thanks for the investigation, this sounds good to me!
There was a problem hiding this comment.
I've removed the first commit. This has the effect that, since getMaxThreadCPUDeltaPerMs no longer looks at the processed CPU deltas, it might return a value that's lower than the maximum cpu delta per ms once the null substitution has happened:
Let's say you have this unprocessed data: [{ timeDelta: 1, cpuDelta: null }, { timeDelta: 10, cpuDelta: 10 }]
Then you replace the null with the 10.
Now the first value has { timeDelta: 1, cpuDelta: 10 }, i.e. 10 CPU per ms. getMaxThreadCPUDeltaPerMs will now just compute 1 CPU per ms.
I think that's ok because this situation is broken anyway.
…rval. This also fixes the situation where computeMaxThreadCPUDeltaPerMs said it should be called after processing the thread CPU deltas, but computeMaxCPUDeltaPerInterval was calling it before processing.
The loop skips i === 0 so the value was never used.
mstange
force-pushed
the
push-puyntwqusqyq
branch
from
1930cbf to
df6d128
Compare
Updates: [Nicolas Chevobbe] Adapt Keyboard shortcut dialog in High Contrast Mode. (#5245) [Nicolas Chevobbe] Fix sidebar-toggle in High Contrast Mode. (#5246) [Nicolas Chevobbe] Fix timeline selection overlay time / hover line in High Contrast Mode (#5247) [Zac Spitzer] fix broken link for processed profile format (#5267) [Greg Tatum] Update the memory allocation docs and add DHAT docs (#5270) [Markus Stange] Simplify some code related to thread CPU deltas (#5265) [Greg Tatum] Update dhat convertor to work better with valgrind (#5269) [Markus Stange] Rename UniqueStringArray to StringTable. (#5283) [Markus Stange] Use snapshot testing in the symbolicator CLI test. (#5284) [Markus Stange] Fix two confused upgraders which didn't expect to be run on the serialized format. (#5285) [Nicolas Chevobbe] Fix timelineSettingsHiddenTracks in High Contrast Mode. (#5250) [Julien Wajsberg] Fix some test and non-test warnings (#5294) [Nisarg Jhaveri] Support importing simpleperf trace files from Android Studio (#5212) [Sean Kim] Add HTTP response status code in the profiler network tab (#5297) [Markus Stange] Change StringTable API a bit. (#5286) [Markus Stange] Correctly declare imported simpleperf trace profiles to be of the current version. (#5312) [Markus Stange] Two small changes (#5313) [Nazım Can Altınova] Show sample tooltips on sample graph hover (#5298) Also thanks to our localizers: en-CA: chutten es-CL: ravmn es-CL: ravmn fr: Théo Chevalier ia: Melo46 ia: Melo46 sv-SE: Andreas Pettersson uk: Lobodzets zh-TW: Olvcpr423
2 participants
There was a mismatch in whether
computeMaxThreadCPUDeltaPerMswas called before or after thread CPU delta processing. It didn't make a difference in practice, but it's better to just always call it with the unprocessed values. This will avoid problems if we end up giving the processed values a different type.