[webkit-reviews] review granted: [Bug 194564] Web Inspector: Better categorize CPU usage per-thread / worker : [Attachment 362201] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 194564: Web Inspector: Better categorize CPU usage per-thread / worker
https://bugs.webkit.org/show_bug.cgi?id=194564

Attachment 362201: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=362201&action=review




--- Comment #14 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 362201
  --> https://bugs.webkit.org/attachment.cgi?id=362201
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=362201&action=review

r=me, thanks for iterating :)

> LayoutTests/inspector/cpu-profiler/threads.html:16
> +	       InspectorProtocol.awaitEvent({event:
"CPUProfiler.trackingStart"}).then((messageObject) => {

Please put the `.then(` on a new line.

> LayoutTests/inspector/cpu-profiler/threads.html:18
> +		   ProtocolTest.expectThat(typeof
messageObject.params.timestamp === "number", "Should have a timestamp when
starting.");

`expectEqual`?

> LayoutTests/inspector/cpu-profiler/threads.html:28
> +		   ProtocolTest.expectThat(typeof messageObject.params.event
=== "object", "Should have an event object.");
> +		   ProtocolTest.expectThat(typeof
messageObject.params.event.timestamp === "number", "Event should have a
timestamp.");
> +		   ProtocolTest.expectThat(typeof
messageObject.params.event.usage === "number", "Event should have a usage.");

`expectEqual`?

> LayoutTests/inspector/cpu-profiler/threads.html:29
> +		   ProtocolTest.expectThat(messageObject.params.event.usage >=
0, "usage should be greater than or equal to zero.");

`expectGreaterThanOrEqual`?

> LayoutTests/inspector/cpu-profiler/threads.html:48
> +		   ProtocolTest.expectThat(others.length > 0, "Event should
have other worker threads.");

`expectGreaterThan`?

> LayoutTests/inspector/cpu-profiler/threads.html:56
> +	       InspectorProtocol.awaitEvent({event:
"CPUProfiler.trackingComplete"}).then((messageObject) => {

Ditto (>16).

> LayoutTests/inspector/cpu-profiler/threads.html:58
> +		   resolve();

Rather than just call `resolve`, you can `.then(resolve, reject)` the entire
event.

> Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp:87
> +	   .setUsage(thread.cpu)

Since you mention that we should never see >100%, should we `ASSERT(thread.cpu
<= 100)`?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:201
> +	   kr = thread_info(sendRight.sendRight(), THREAD_BASIC_INFO,
reinterpret_cast<thread_info_t>(&threadInfo), &threadInfoCount);

NIT: `sendRight.sendRight` is slightly annoying to read, so maybe we can follow
what used to be there and rename `MachSendRight sendRight` to `MachSendRight
machThread`?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:209
> +	   kr = thread_info(sendRight.sendRight(), THREAD_IDENTIFIER_INFO,
reinterpret_cast<thread_info_t>(&threadIdentifierInfo),
&threadIdentifierInfoCount);
> +	   if (kr != KERN_SUCCESS)
> +	       continue;

Is it critical that we have `threadIdentifierInfo` in order to show any CPU
usage?	Is there nothing we can show for this thread (e.g. some sort of
"unknown") without this data?  If so, I think we shouldn't `continue` here.  If
this is unlikely to happen, then maybe an `ASSERT` at the very least.

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:215
> +	   kr = thread_info(sendRight.sendRight(), THREAD_EXTENDED_INFO,
reinterpret_cast<thread_info_t>(&threadExtendedInfo),
&threadExtendedInfoCount);
> +	   if (kr != KERN_SUCCESS)
> +	       continue;

Ditto (>207).

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:220
> +	       usage = threadBasicInfo->cpu_usage /
static_cast<float>(TH_USAGE_SCALE) * 100.0;

Ditto (>InspectorCPUProfilerAgent.cpp: 87).

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:229
> +	   infos.append(ThreadInfo { WTFMove(sendRight), usage, threadName,
dispatchQueueName });

If you're using uniform initialization, either drop the `ThreadInfo` or replace
it entirely with the specified constructor (e.g.
`ThreadInfo(WTFMove(sendRight), usage, threadName, dispatchQueueName)`).

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:296
> +	   if (thread.threadName == "JavaScriptCore bmalloc scavenger")

Is there any way we can get this name without hardcoding it?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:310
> +	   data.cpu += thread.usage;
> +	   if (isDebuggerThread(thread))

Style: missing newline.

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:317
> +	       data.cpuThreads.append(ThreadCPUInfo { "Main Thread"_s,
String(), thread.usage, ThreadCPUInfo::Type::Main});

Ditto (>229).

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:324
> +	   data.cpuThreads.append(ThreadCPUInfo { thread.threadName,
threadIdentifier, thread.usage, type });

Ditto (>229).

> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:49
> +		   console.assert(!this._mainThreadUsage);

This could use a message like "There should only be one main thread.".