core(tracehouse): use tasks to improve profiler timing data #11446
Conversation
patrickhulce
requested review from
paulirish
and removed request for
a team
patrickhulce
force-pushed
the
cpu_profiler_task_conflicts_part_a
branch
from
71ca472
to
4d94142
Compare
| * @param {{printWidth: number, startTime?: number, endTime?: number, taskLabelFn?: (node: TaskNode) => string}} options | ||
| * @return {string} | ||
| */ | ||
| static printTaskTreeToDebugString(tasks, options) { |
There was a problem hiding this comment.
its not the biggest function but i'm wondering if we should put this (and our other debug-only functions) in separate modules to we can easily exclude them from our browserify bundles....
There was a problem hiding this comment.
My initial impression is that it doesn't seem worth it to move away from the place that the debugging helps, but a followup for relocating all of our debugging utils sgtm
| @@ -65,6 +65,40 @@ describe('CPU Profiler Model', () => { | |||
| ]); | |||
| }); | |||
|
|
|||
| it('should create events while aware of other tasks', () => { | |||
There was a problem hiding this comment.
fyi there may be some test case here or nearby that are useful: https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/web_tests/http/tests/devtools/profiler/cpu-profiler-fix-missing-samples.js
There was a problem hiding this comment.
sweet thanks, when we get to the skipped sample logic we'll definitely incorporate those 👍
| * @param {{earliestPossibleTimestamp: number, latestPossibleTimestamp: number, lastKnownTaskStartTimeIndex: number, lastKnownTaskEndTimeIndex: number, knownTasksByStartTime: Array<{startTime: number, endTime: number}>, knownTasksByEndTime: Array<{startTime: number, endTime: number}>}} data | ||
| * @return {{timestamp: number, lastStartTimeIndex: number, lastEndTimeIndex: number}} | ||
| */ | ||
| _findEffectiveTimestamp(data) { |
There was a problem hiding this comment.
there's some decent complexity here and i feel odd having it untested.
i'm wondering if we could test this more clearly with some dedicated _findEffectiveTimestamp() unit tests or doing it higher up at either the cpu profiler model level or the main thread tasks level
There was a problem hiding this comment.
doing it higher up at either the cpu profiler model level or the main thread tasks level
that's exactly what the unit test added in this PR does ;)
I can add explicit unit tests for this private method as well if you think the current cases aren't sufficient 👍
| * @param {Array<{startTime: number, endTime: number}>} knownTasks | ||
| * @param {{type: 'startTime'|'endTime', lastKnownTaskIndex: number, earliestPossibleTimestamp: number, latestPossibleTimestamp: number}} options | ||
| */ | ||
| _getTasksInRange(knownTasks, options) { |
There was a problem hiding this comment.
again, personal taste, but i figure these helper methods are better if defined after their use. makes reading the file (or reviewing) from top to bottom a little more straightforward.
There was a problem hiding this comment.
would you like all of them moved or just the new ones? I'm a little concerned about muddying the diff with a move of all private methods here (and a little concerned about the consistency if we only move the new ones)
| knownTasksByEndTime, | ||
| }); | ||
|
|
||
| profilerTimestamp += timeDelta; |
There was a problem hiding this comment.
why do we mutate this one? seems odd that it would be changing but maybe i'm misunderstanding its use.
There was a problem hiding this comment.
this is just a rename of the old timestamp which is the raw timestamp of a sample as reported by the profiler's timeDeltas, but it sounds like the new name made that less clear to you :)
currentProfilerTimestamp
currentTimestampAsReportedByProfiler
rawTimestamp
currentRawTimestamp
like any of those better?
There was a problem hiding this comment.
went with currentProfilerTimestamp
Co-authored-by: Paul Irish <paulirish@google.com>
Summary
The next phase of CPU profiler work got really out of hand really quickly, so stopping at a reasonable checkpoint. This PR improves the timing of sampling profiler data using other task information from a trace. The sampling profiler is still out of the normal flow of Lighthouse, so the test cases are the only actual usage.
Also a nice juicy debug function for task data which should help generate those nice "artistic" rendering of traces :)
Related Issues/PRs
ref #8526