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
<rdar://problem/47532910>
Created attachment 360048 [details] [PATCH] Proposed Fix
Created attachment 360058 [details] [PATCH] Proposed Fix Should apply now.
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.
Created attachment 360077 [details] [PATCH] Proposed Fix Oops rebaseline troubles.
Created attachment 360078 [details] [PATCH] Proposed Fix
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.
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 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.
Created attachment 360156 [details] [PATCH] Proposed Fix
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"?
Created attachment 360176 [details] [PATCH] For Landing
Comment on attachment 360176 [details] [PATCH] For Landing Clearing flags on attachment: 360176 Committed r240522: <https://trac.webkit.org/changeset/240522>
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!