Compute cpuRatio values when computing the derived thread#5288
Conversation
128fdbf to
1f72810
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5288 +/- ##
==========================================
- Coverage 85.93% 85.93% -0.01%
==========================================
Files 311 312 +1
Lines 29812 29771 -41
Branches 8222 8196 -26
==========================================
- Hits 25620 25583 -37
+ Misses 3601 3597 -4
Partials 591 591 ☔ View full report in Codecov by Sentry. |
1f72810 to
22e9b62
Compare
e5178f9 to
7aeaf62
Compare
2fd1aaf to
edb7649
Compare
edb7649 to
fa5b453
Compare
dac5897 to
254026d
Compare
canova
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks for the changes.
| if (fullThreadSample + 1 < fullThreadSamplesCPURatio.length) { | ||
| afterSampleCpuRatio = fullThreadSamplesCPURatio[fullThreadSample + 1]; | ||
| } |
There was a problem hiding this comment.
For the last sample, afterSampleCpuRatio will be always 1, right? Is this correct or do we maybe want to use the same value as beforeSampleCpuRatio?
There was a problem hiding this comment.
I'll do what you suggested. I don't think there's a clear answer because we just don't have the information about what happens after the last sample.
There was a problem hiding this comment.
I think we set the "after last" duration to the interval, so it's true that it would be weird to make it look as if there was 100% CPU for one interval after the last sample. Defaulting to zero would make more sense. But defaulting to beforeSampleCpuRatio also sounds fine.
There was a problem hiding this comment.
Oh, using beforeSampleCpuRatio is also what we do in _accumulateSampleCategories, so yes this would make hit testing consistent with rendering.
| 'Unhandled threadCPUDelta unit in the processing.' | ||
| ); | ||
| const threadCPURatio: Float64Array = new Float64Array(threadCPUDelta.length); | ||
| threadCPURatio[0] = 0; |
There was a problem hiding this comment.
Hm why do we zero out the first value? I vaguely remember something related to that but forgot. It might be good to add a comment about it as well.
There was a problem hiding this comment.
Hmm I thought I had written a comment somewhere but I can't find it now. Yes I'll add a comment.
Changing this as follows:
diff --git a/src/profile-logic/cpu.js b/src/profile-logic/cpu.js
index 24eb84e7aa..a16a394ad7 100644
--- a/src/profile-logic/cpu.js
+++ b/src/profile-logic/cpu.js
@@ -123,8 +123,13 @@
}
const threadCPURatio: Float64Array = new Float64Array(threadCPUDelta.length);
+
+ // Ignore threadCPUDelta[0] and set threadCPURatio[0] to zero - there is no
+ // previous sample so there is no meaningful value we could compute here.
threadCPURatio[0] = 0;
+ // For the rest of the samples, compute the ratio based on the CPU delta and
+ // on the elapsed time between samples (timeDeltas[i]).
for (let i = 1; i < threadCPUDelta.length; i++) {
const referenceCpuDelta = maxThreadCPUDeltaPerMs * timeDeltas[i];
const cpuDelta = threadCPUDelta[i];This avoids a circular dependency between cpu.js, process-profile.js and profile-data.js, because in the next commit I want to use a function from cpu.js in profile-data.js.
It looks like I forgot to update this comment when I changed this function to no longer use the interval.
254026d to
3f0926c
Compare
This makes it clearer that, with time-based CPU deltas, you can have profiles that never hit 100% CPU usage.
3f0926c to
748d969
Compare
Updates: [Julien Wajsberg] Some more small refactorings (#5320) [Markus Stange] Pass the correct sample index offset to getTimingsForCallNodeIndex for the flame graph tooltip. (#5328) [Nisarg Jhaveri] Update docs to include Android Studio/Simpleperf trace file support (#5309) [Markus Stange] Don't pass the preview filtered thread to getTimingsForPath/CallNodeIndex. (#5329) [Nazım Can Altınova] Add a "Sample timestamp" field to the sample tooltip in timeline (#5322) [Markus Stange] Reduce confusion between call tree summary strategy aware samples and regular samples (#5330) [Markus Stange] Rename this getCounter selector to getCounters. (#5337) [Markus Stange] Make sample indexes compatible between the unfiltered and (preview) filtered call tree summary strategy samples when using an allocation strat> [Markus Stange] Remove some code that uses the preview filtered thread (#5336) [Markus Stange] Remove getMarkerSchemaName special cases - look up marker schemas from data.type and nothing else (#5293) [Markus Stange] Remove the makeProfileSerializable step - make the raw in-memory profile match the format that's stored in the file (#5287) [Nicolas Chevobbe] Adapt FilterNavigatorBar to High Contrast Mode. (#5257) [Nicolas Chevobbe] Adapt Tracks to High Contrast Mode. (#5252) [Markus Stange] Adjust string index fields in markers when merging threads (#5344) [Theodoros Nikolaou] Localize title and aria label in ProfileName (#5345) [Julien Wajsberg] Adapt time-slice selection in High Contrast Mode. (#5259) [Markus Stange] Make stackTable (sub)category derived data (#5342) [Markus Stange] Compute cpuRatio values when computing the derived thread (#5288) [Nazım Can Altınova] Add a context menu item to open the JS scripts in DevTools debugger (#5295) Also thanks to our localizers: el: Jim Spentzos fr: Théo Chevalier it: Francesco Lodolo [:flod] zh-TW: Pin-guang Chen
Production | Deploy preview
processThreadCPUDeltamakes it so that thethreadCPUDeltacolumn doesn't have any null values. However, the type of thethreadCPUDeltacolumn still looks as if values could be null.Now that we have a derived
Threadtype, we can change the type so that it expresses that the values can't be null. So we move the call toprocessThreadCPUDeltaa bit earlier in the derivation pipeline so that all the types work out.Even better, we can compute CPU ratios at this point, and not even store the "clamped and non-null cpu delta" values.
And since we're doing that, I'm changing the behavior for null cpu delta values: Rather than using the closest non-null cpu delta and spreading it over a potentially-different sample time delta, just set the cpu ratio to 1. The only known cause of null values (other than the first value, which we ignore) is the base profiler, and we want CPU percentage to be reported as 100% during the base profiler time.
Fixes #4072.