Web Inspector: Better categorize CPU usage per-thread / worker

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Better categorize CPU usage per-thread / worker

Web Inspector's CPU Usage Timeline would like to break out CPU usage per worker, on the main thread, and "other".

[Joseph Pecoraro]

Created attachment 361865 [details]
[PATCH] Proposed Fix

This should require skipping the inspector/cpu-profiler/worker-threads.html test on some linux ports. Lets see what the bots say.

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[EWS Watchlist]

Attachment 361865 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ResourceUsageData.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 361865 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/protocol/CPUProfiler.json:17
>              "id": "Event",

An alternative possibility is just sending a near complete list of all threads to the inspector and letting it group / sort through them as it wishes. Something like a:

    {
        "id": "ThreadInfo",
        "properties": [
            { "name": "name", "type": "string", "description": "Some thread identification information." },
            { "name": "usage", "type": "number" },
            { "name": "targetId", "type": "string", "optional": true }
        ]
    }
    ...
    { "name": "threads", "type": "array", "items": { "$ref": "ThreadInfo" }, "description": "Per-thread CPU usage information" },

[Devin Rousso]

Comment on attachment 361865 [details]
[PATCH] Proposed Fix

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

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

If you're "skipping" the event, we should ignore the data, so please remove this `messageObject`.

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

NIT: rather than nesting this `.then()`, you could just return the `awaitEvent` (e.g. promise chaining).

>> Source/JavaScriptCore/inspector/protocol/CPUProfiler.json:17
>>              "id": "Event",
> 
> An alternative possibility is just sending a near complete list of all threads to the inspector and letting it group / sort through them as it wishes. Something like a:
> 
>     {
>         "id": "ThreadInfo",
>         "properties": [
>             { "name": "name", "type": "string", "description": "Some thread identification information." },
>             { "name": "usage", "type": "number" },
>             { "name": "targetId", "type": "string", "optional": true }
>         ]
>     }
>     ...
>     { "name": "threads", "type": "array", "items": { "$ref": "ThreadInfo" }, "description": "Per-thread CPU usage information" },

I like this approach better.  It's more future-compatible 😁

You could also a `type` that could indicate the thread type (in the case that a worker thread has a specified name that doesn't include "worker" in it).

> Source/JavaScriptCore/inspector/protocol/CPUProfiler.json:23
> +                { "name": "webkitThreadUsage", "type": "number", "optional": true, "description": "Usage by WebKit specific threads. GC Threads, Compiler Threads, etc." },

NIT: I'd put the examples within "(...)" and have the `description` be one sentence.

> Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp:91
> +        event->setMainThreadUsage(*data.cpuMainThread);

When using `Optional`s, I prefer calling `.value()` over `*` to retrieve the value.  This makes it clearer that the value is an optional, not a pointer.

> Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp:94
> +        event->setWebkitThreadUsage(*data.cpuWebKitThreads);

Ditto (>91).

> Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp:97
> +        RefPtr<JSON::ArrayOf<Protocol::CPUProfiler::WorkerCPUUsage>> workerThreads = JSON::ArrayOf<Protocol::CPUProfiler::WorkerCPUUsage>::create();

`auto`?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:-157
> -    kr = vm_deallocate(mach_task_self(), (vm_offset_t)threadList, threadCount * sizeof(thread_t));
> -    ASSERT(kr == KERN_SUCCESS);

This didn't get "copied" anywhere.  Is it needed?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:187
> +static Vector<ThreadInfo> threadInfos()

"Infos" is a bit weird, but I can't think of anything better given that the individual item is `ThreadInfo` :(

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:197
> +        MachSendRight sendRight = MachSendRight::adopt(threadList[i]);

`auto`?

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

`kr` was already defined above, so either remove the `auto` or rename it.

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:209
> +        if (kr != KERN_SUCCESS)
> +            continue;

Should we bail if we aren't able to get the identifier info?  Is there value in thread info that lacks identifier info?  Can we still do something for non-identifiable threads (e.g. an "Other" usage category)?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:215
> +        if (kr != KERN_SUCCESS)
> +            continue;

Ditto (>208).

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:223
> +        String dispatchQueueName = String();

Is the `String()` necessary?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:225
> +            dispatch_queue_t queue = *(dispatch_queue_t *)threadIdentifierInfo.dispatch_qaddr;

`reinterpret_cast<dispatch_queue_t*>`?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:314
> +        data.cpu += thread.usage;

Style: missing newline after this line.

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:316
> +        if (isDebuggerThread(thread))
> +            continue;

Does this mean that there is no "debugger worker thread", or "debugger webkit thread"?  Should we not be `continue`ing and instead only set `data.cpuExcludingDebuggerThreads` when `!isDebuggerThread(thread)`?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:326
> +            data.cpuWebKitThreads = *data.cpuWebKitThreads + thread.usage;

Ditto (>InspectorCPUProfilerAgent.cpp:91).

> Source/WebCore/workers/WorkerThread.cpp:56
> +HashSet<WorkerThread*>& WorkerThread::workerThreads(const LockHolder&)

Is having `const LockHolder&` as a parameter purely to ensure that callers have acquired the lock?

> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:28
> +    constructor({timestamp, usage, mainThreadUsage, webkitThreadUsage, workerThreads})

Should we add a `= {}` just in case we don't pass an object into this (I don't think that that's possible, but just in case)?

> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:44
> +        this._workerThreadUsage = this._workers.reduce((total, worker) => total += worker.usage, 0);

We should just use `+`, not `+=`, especially since we're returning the sum (e.g. `total` doesn't persist).

> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:48
> +        console.assert(this._unknownThreadUsage >= 0 || this._unknownThreadUsage >= -0.0001);

-Number.EPSILON?  Is it necessary to have both checks, given that one entirely encompasses the other?

> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:66
> +    get mainThreadUsage() { return this._mainThreadUsage; }
> +    get webkitThreadUsage() { return this._webkitThreadUsage; }
> +    get workerThreadUsage() { return this._workerThreadUsage; }
> +    get unknownThreadUsage() { return this._unknownThreadUsage; }
> +    get workers() { return this._workers; }

Are these values supposed to be used in this patch, or is this patch purely about sending more data to the frontend?

[Joseph Pecoraro]

Comment on attachment 361865 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/cpu-profiler/worker-threads.html:23
>> +                InspectorProtocol.awaitEvent({event: "CPUProfiler.trackingUpdate"}).then((messageObject) => {
> 
> NIT: rather than nesting this `.then()`, you could just return the `awaitEvent` (e.g. promise chaining).

Sounds good!

>> Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp:91
>> +        event->setMainThreadUsage(*data.cpuMainThread);
> 
> When using `Optional`s, I prefer calling `.value()` over `*` to retrieve the value.  This makes it clearer that the value is an optional, not a pointer.

That isn't the normal style across WebKit, at least that I've seen.

>> Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp:97
>> +        RefPtr<JSON::ArrayOf<Protocol::CPUProfiler::WorkerCPUUsage>> workerThreads = JSON::ArrayOf<Protocol::CPUProfiler::WorkerCPUUsage>::create();
> 
> `auto`?

Needed for the RefPtr<> vs Ref<> confusion.

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:-157
>> -    ASSERT(kr == KERN_SUCCESS);
> 
> This didn't get "copied" anywhere.  Is it needed?

Oops, yes the vm_deallocate and ASSERT is definitely needed!

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:209
>> +            continue;
> 
> Should we bail if we aren't able to get the identifier info?  Is there value in thread info that lacks identifier info?  Can we still do something for non-identifiable threads (e.g. an "Other" usage category)?

This shouldn't happen, and we will assert as such.

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:316
>> +            continue;
> 
> Does this mean that there is no "debugger worker thread", or "debugger webkit thread"?  Should we not be `continue`ing and instead only set `data.cpuExcludingDebuggerThreads` when `!isDebuggerThread(thread)`?

All of the other values (cpuWorkerThreads, cpuWebKitThreads) exclude debugger threads.

>> Source/WebCore/workers/WorkerThread.cpp:56
>> +HashSet<WorkerThread*>& WorkerThread::workerThreads(const LockHolder&)
> 
> Is having `const LockHolder&` as a parameter purely to ensure that callers have acquired the lock?

Yep

>> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:28
>> +    constructor({timestamp, usage, mainThreadUsage, webkitThreadUsage, workerThreads})
> 
> Should we add a `= {}` just in case we don't pass an object into this (I don't think that that's possible, but just in case)?

I'd rather this be an error, given some values are non-optional (and asserted).

>> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:48
>> +        console.assert(this._unknownThreadUsage >= 0 || this._unknownThreadUsage >= -0.0001);
> 
> -Number.EPSILON?  Is it necessary to have both checks, given that one entirely encompasses the other?

Given there are multiple numbers in the math above the slide could be more than a single EPSILON. But yes I'll reduce the assertion.

>> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:66
>> +    get workers() { return this._workers; }
> 
> Are these values supposed to be used in this patch, or is this patch purely about sending more data to the frontend?

They will be used by later patches.

[Joseph Pecoraro]

Created attachment 361954 [details]
[PATCH] Proposed Fix

I'll come back around to sending complete ThreadInfo objects to the frontend, but this addresses the other comments.

[Joseph Pecoraro]

Created attachment 362194 [details]
[PATCH] Proposed Fix

This sends the frontend per-Thread CPU info.

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[EWS Watchlist]

Comment on attachment 362194 [details]
[PATCH] Proposed Fix

Attachment 362194 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11167955

New failing tests:
jquery/offset.html

[EWS Watchlist]

Created attachment 362200 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Joseph Pecoraro]

Comment on attachment 362194 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/cpu-profiler/threads.html:12
> +    ProtocolTest.debug();

Oops.

[Joseph Pecoraro]

Created attachment 362201 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 362201 [details]
[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.".

[Joseph Pecoraro]

Created attachment 362320 [details]
[PATCH] For Landing

[Joseph Pecoraro]

Comment on attachment 362201 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/cpu-profiler/threads.html:58
>> +                resolve();
> 
> Rather than just call `resolve`, you can `.then(resolve, reject)` the entire event.

These are all styles. We've been lenient on style in tests and I'm going to stick with my style choices for tests because I find them more readable, but if we decide to merge on specific styles in our tests I can reconsider.

>> 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)`?

Done.

>> 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`?

I've considered this. I think `machThread()` would be a nicer name but I'd have to ask folks that have more experience with Mach APIs. "Send right" might be the name people expect given how I've heard experts talking in this area.

>> 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)`).

I'm going to keep the `ThreadInfo` prefix, but drop the unnecessary constructor. Thanks!

>> 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?

We would need to expose it from bmalloc...

>> Source/WebInspectorUI/UserInterface/Models/CPUTimelineRecord.js:49
>> +                console.assert(!this._mainThreadUsage);
> 
> This could use a message like "There should only be one main thread.".

Good idea.

[WebKit Commit Bot]

Comment on attachment 362320 [details]
[PATCH] For Landing

Clearing flags on attachment: 362320

Committed r241739: <https://trac.webkit.org/changeset/241739>

[Radar WebKit Bug Importer]

<rdar://problem/48180069>

[Frédéric Wang (:fredw)]

Comment on attachment 362320 [details]
[PATCH] For Landing

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

> Source/WebCore/page/ResourceUsageData.h:83
> +    String identifier;

I think this needs to include WTFString.h or (forward declare String if that's possible) otherwise build might fail when unified source rotates.

> Source/WebCore/page/ResourceUsageData.h:93
> +    Vector<ThreadCPUInfo> cpuThreads;

Ditto for Vector.h

[Frédéric Wang (:fredw)]

Committed r242466: <https://trac.webkit.org/changeset/242466>