Bug 193796

Summary: Web Inspector: Exclude Debugger Threads from CPU Usage values in Web Inspector
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, drousso, ews-watchlist, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix
none
[PATCH] For Landing none

Description Joseph Pecoraro 2019-01-24 16:12:01 PST
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
Comment 1 Radar WebKit Bug Importer 2019-01-24 16:12:27 PST
<rdar://problem/47532910>
Comment 2 Joseph Pecoraro 2019-01-24 16:39:15 PST
Created attachment 360048 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2019-01-24 17:21:16 PST
Created attachment 360058 [details]
[PATCH] Proposed Fix

Should apply now.
Comment 4 EWS Watchlist 2019-01-24 17:26:09 PST Comment hidden (obsolete)
Comment 5 Joseph Pecoraro 2019-01-24 19:51:33 PST
Created attachment 360077 [details]
[PATCH] Proposed Fix

Oops rebaseline troubles.
Comment 6 Joseph Pecoraro 2019-01-24 19:52:40 PST
Created attachment 360078 [details]
[PATCH] Proposed Fix
Comment 7 EWS Watchlist 2019-01-24 19:54:05 PST Comment hidden (obsolete)
Comment 8 Devin Rousso 2019-01-24 22:51:33 PST
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
    }
Comment 9 Joseph Pecoraro 2019-01-25 13:47:15 PST
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.
Comment 10 Joseph Pecoraro 2019-01-25 13:47:30 PST
Created attachment 360156 [details]
[PATCH] Proposed Fix
Comment 11 Devin Rousso 2019-01-25 13:55:49 PST
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"?
Comment 12 Joseph Pecoraro 2019-01-25 15:27:39 PST
Created attachment 360176 [details]
[PATCH] For Landing
Comment 13 WebKit Commit Bot 2019-01-25 15:45:10 PST
Comment on attachment 360176 [details]
[PATCH] For Landing

Clearing flags on attachment: 360176

Committed r240522: <https://trac.webkit.org/changeset/240522>
Comment 14 Joseph Pecoraro 2019-03-29 19:30:23 PDT
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!