Bug 193796 – Web Inspector: Exclude Debugger Threads from CPU Usage values in Web Inspector

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Exclude Debugger Threads from CPU Usage values in Web Inspector

When the Web Inspector is inspecting the CPU usage of a Web Content process, we should exclude Web Inspector related work:

  • Sampling Profiler when the Scripts timeline is shown
  • Resource Usage Thread itself which is calculating the cpu usage and memory usage of the process

[Radar WebKit Bug Importer]

<rdar://problem/47532910>

[Joseph Pecoraro]

Created attachment 360048 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 360058 [details]
[PATCH] Proposed Fix

Should apply now.

[EWS Watchlist]

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


ERROR: Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:143:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 9 files


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

[Joseph Pecoraro]

Created attachment 360077 [details]
[PATCH] Proposed Fix

Oops rebaseline troubles.

[Joseph Pecoraro]

Created attachment 360078 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

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


ERROR: Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:143:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 9 files


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

[Devin Rousso]

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

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

> Source/WebCore/ChangeLog:17
> +        For OS(DARWIN) ports, when starting to observe resource usage

Grammar: there should be a comma => "usage,\n grab"

> Source/WebCore/ChangeLog:19
> +        in a thread safe way. For our purposes, Web Inspector timelines,

NIT: "purposes, Web Inspector timelines,\n this" => "purposes (WebInspector timelines),\n this"

> Source/WebCore/ChangeLog:20
> +        this will be good enough. The SamplingProfiler won't change

Good enough for what?  Good enough for ensuring that we exclude debugger threads?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:143
> +static Vector<MachSendRight> filterThreads(Function<bool (mach_port_t)>&& filterFunction)

Is this the best name for this?  I feel like the old `threadSendRights` was a bit clearer as to what this function actually did.  Maybe we should just have a better name for `filterFunction`, like `sendPredicate`?

Alternatively, couldn't you filter the list previously returned by `threadSendRights` after it's been called (create a `Vector::copyMatching`), this way we can avoid some of the other logic in this function (e.g. `threadSendRights` is guaranteed to be a superset of `threadSendRightsExcludingDebuggerThreads`, so we shouldn't have to call `task_threads` again)?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:170
> +static Vector<MachSendRight> threadSendRightsExcludingDebuggerThreads(mach_port_t resourceUsageThread, mach_port_t samplingProfilerThread)

It seems odd to have a variable named `samplingProfilerThread` in the case that the `SAMPLING_PROFILER` flag is disabled.  Why not use a `HashSet` (or even a struct with `if`-wrapped member variables) so that it can be more flexible with other thread types in the future?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:181
> +static float cpuUsage(Vector<MachSendRight> machThreads)

NIT: should this be a & to avoid copying the vector?

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:227
> +#if ENABLE(SAMPLING_PROFILER)

NIT: shouldn't only the `m_samplingProfilerMachThread = ...` line be wrapped in the `#if`?  This would let you avoid defining the function twice.

    void ResourceUsageThread::platformSaveStateBeforeStarting()
    {
    #if ENABLE(SAMPLING_PROFILER)
        m_samplingProfilerMachThread = m_vm->samplingProfiler() ? m_vm->samplingProfiler()->machThread() : MACH_PORT_NULL;
    #endif
    }

[Joseph Pecoraro]

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

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

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:143
>> +static Vector<MachSendRight> filterThreads(Function<bool (mach_port_t)>&& filterFunction)
> 
> Is this the best name for this?  I feel like the old `threadSendRights` was a bit clearer as to what this function actually did.  Maybe we should just have a better name for `filterFunction`, like `sendPredicate`?
> 
> Alternatively, couldn't you filter the list previously returned by `threadSendRights` after it's been called (create a `Vector::copyMatching`), this way we can avoid some of the other logic in this function (e.g. `threadSendRights` is guaranteed to be a superset of `threadSendRightsExcludingDebuggerThreads`, so we shouldn't have to call `task_threads` again)?

That sounds good.

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:181
>> +static float cpuUsage(Vector<MachSendRight> machThreads)
> 
> NIT: should this be a & to avoid copying the vector?

Yep!

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:227
>> +#if ENABLE(SAMPLING_PROFILER)
> 
> NIT: shouldn't only the `m_samplingProfilerMachThread = ...` line be wrapped in the `#if`?  This would let you avoid defining the function twice.
> 
>     void ResourceUsageThread::platformSaveStateBeforeStarting()
>     {
>     #if ENABLE(SAMPLING_PROFILER)
>         m_samplingProfilerMachThread = m_vm->samplingProfiler() ? m_vm->samplingProfiler()->machThread() : MACH_PORT_NULL;
>     #endif
>     }

Will do.

[Joseph Pecoraro]

Created attachment 360156 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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

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

r=me

> Source/WebCore/ChangeLog:20
> +        this will be good enough to threading identifying the

Typo: "threading identifying"?

[Joseph Pecoraro]

Created attachment 360176 [details]
[PATCH] For Landing

[WebKit Commit Bot]

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

Clearing flags on attachment: 360176

Committed r240522: <https://trac.webkit.org/changeset/240522>

[Joseph Pecoraro]

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

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

> Source/WebCore/page/ResourceUsageThread.h:77
> +    void platformSaveStateBeforeStarting();

Somehow this patch never called this. I must have lost it when cherry-picking changes!