Attachment 360078 Details for Bug 193796 – [PATCH] Proposed Fix

[patch] [PATCH] Proposed Fix

cpu-5.patch (text/plain), 9.95 KB, created by Joseph Pecoraro on 2019-01-24 19:52:40 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Joseph Pecoraro

Created: 2019-01-24 19:52:40 PST

Size: 9.95 KB

patch

obsolete

>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index f9257159ab0..c32711bdcd9 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-01-24  Joseph Pecoraro  <pecoraro@apple.com>
>+
>+        Web Inspector: Exclude Debugger Threads from CPU Usage values in Web Inspector
>+        https://bugs.webkit.org/show_bug.cgi?id=193796
>+        <rdar://problem/47532910>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * runtime/SamplingProfiler.cpp:
>+        (JSC::SamplingProfiler::machThread):
>+        * runtime/SamplingProfiler.h:
>+        Expose the mach_port_t of the SamplingProfiler thread
>+        so it can be tested against later.
>+
> 2019-01-24  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] ErrorConstructor should not have own IsoSubspace
>diff --git a/Source/JavaScriptCore/runtime/SamplingProfiler.cpp b/Source/JavaScriptCore/runtime/SamplingProfiler.cpp
>index f4b07d9ef82..07df96681d3 100644
>--- a/Source/JavaScriptCore/runtime/SamplingProfiler.cpp
>+++ b/Source/JavaScriptCore/runtime/SamplingProfiler.cpp
>@@ -1079,6 +1079,16 @@ void SamplingProfiler::reportTopBytecodes(PrintStream& out)
>     }
> }
> 
>+#if OS(DARWIN)
>+mach_port_t SamplingProfiler::machThread()
>+{
>+    if (!m_thread)
>+        return MACH_PORT_NULL;
>+
>+    return m_thread->machThread();
>+}
>+#endif
>+
> } // namespace JSC
> 
> namespace WTF {
>diff --git a/Source/JavaScriptCore/runtime/SamplingProfiler.h b/Source/JavaScriptCore/runtime/SamplingProfiler.h
>index c79ae7595f2..ffc91357f8d 100644
>--- a/Source/JavaScriptCore/runtime/SamplingProfiler.h
>+++ b/Source/JavaScriptCore/runtime/SamplingProfiler.h
>@@ -182,6 +182,10 @@ public:
>     JS_EXPORT_PRIVATE void reportTopBytecodes();
>     JS_EXPORT_PRIVATE void reportTopBytecodes(PrintStream&);
> 
>+#if OS(DARWIN)
>+    JS_EXPORT_PRIVATE mach_port_t machThread();
>+#endif
>+
> private:
>     void createThreadIfNecessary(const AbstractLocker&);
>     void timerLoop();
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 3e896236d46..ac6ba8e29aa 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,38 @@
>+2019-01-24  Joseph Pecoraro  <pecoraro@apple.com>
>+
>+        Web Inspector: Exclude Debugger Threads from CPU Usage values in Web Inspector
>+        https://bugs.webkit.org/show_bug.cgi?id=193796
>+        <rdar://problem/47532910>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * page/ResourceUsageData.h:
>+        * inspector/agents/InspectorCPUProfilerAgent.cpp:
>+        (WebCore::InspectorCPUProfilerAgent::collectSample):
>+        Show the CPU usage without debugger threads in the Web Inspector's timeline.
>+
>+        * page/ResourceUsageThread.h:
>+        * page/cocoa/ResourceUsageThreadCocoa.mm:
>+        (WebCore::ResourceUsageThread::platformSaveStateBeforeStarting):
>+        For OS(DARWIN) ports, when starting to observe resource usage
>+        grab the mach_port_t of SamplingProfiler on the main thread
>+        in a thread safe way. For our purposes, Web Inspector timelines,
>+        this will be good enough. The SamplingProfiler won't change
>+        during a timeline recording and recording start/stops will never
>+        miss the SamplingProfiler changing.
>+
>+        (WebCore::filterThreads):
>+        (WebCore::threadSendRights):
>+        (WebCore::threadSendRightsExcludingDebuggerThreads):
>+        (WebCore::cpuUsage):
>+        (WebCore::ResourceUsageThread::platformCollectCPUData):
>+        Calculate CPU usage twice, the second time excluding some threads.
>+
>+        * page/linux/ResourceUsageThreadLinux.cpp:
>+        (WebCore::ResourceUsageThread::platformSaveStateBeforeStarting):
>+        (WebCore::ResourceUsageThread::platformCollectCPUData):
>+        Stubs for linux ports.
>+
> 2019-01-24  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: CPU Usage Timeline
>diff --git a/Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp b/Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp
>index 66248b982f2..8dab1afcc97 100644
>--- a/Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp
>+++ b/Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp
>@@ -84,7 +84,7 @@ void InspectorCPUProfilerAgent::collectSample(const ResourceUsageData& data)
> {
>     auto event = Protocol::CPUProfiler::Event::create()
>         .setTimestamp(m_environment.executionStopwatch()->elapsedTimeSince(data.timestamp).seconds())
>-        .setUsage(data.cpu)
>+        .setUsage(data.cpuExcludingDebuggerThreads)
>         .release();
> 
>     m_frontendDispatcher->trackingUpdate(WTFMove(event));
>diff --git a/Source/WebCore/page/ResourceUsageData.h b/Source/WebCore/page/ResourceUsageData.h
>index ab977b5e443..12be8719ea3 100644
>--- a/Source/WebCore/page/ResourceUsageData.h
>+++ b/Source/WebCore/page/ResourceUsageData.h
>@@ -75,6 +75,7 @@ struct ResourceUsageData {
>     constexpr ResourceUsageData() = default;
> 
>     float cpu { 0 };
>+    float cpuExcludingDebuggerThreads { 0 };
>     size_t totalDirtySize { 0 };
>     size_t totalExternalSize { 0 };
>     std::array<MemoryCategoryInfo, MemoryCategory::NumberOfCategories> categories { {
>diff --git a/Source/WebCore/page/ResourceUsageThread.h b/Source/WebCore/page/ResourceUsageThread.h
>index 7dfb4a7bc13..35ae3f03be3 100644
>--- a/Source/WebCore/page/ResourceUsageThread.h
>+++ b/Source/WebCore/page/ResourceUsageThread.h
>@@ -37,6 +37,10 @@
> #include <wtf/Noncopyable.h>
> #include <wtf/Threading.h>
> 
>+#if OS(DARWIN)
>+#include <mach/mach.h>
>+#endif
>+
> namespace JSC {
> class VM;
> }
>@@ -70,6 +74,7 @@ private:
>     void createThreadIfNeeded();
>     void threadBody();
> 
>+    void platformSaveStateBeforeStarting();
>     void platformCollectCPUData(JSC::VM*, ResourceUsageData&);
>     void platformCollectMemoryData(JSC::VM*, ResourceUsageData&);
> 
>@@ -82,6 +87,11 @@ private:
>     // Platforms may need to access some data from the common VM.
>     // They should ensure their use of the VM is thread safe.
>     JSC::VM* m_vm { nullptr };
>+
>+#if ENABLE(SAMPLING_PROFILER) && OS(DARWIN)
>+    mach_port_t m_samplingProfilerMachThread { MACH_PORT_NULL };
>+#endif
>+
> };
> 
> #if PLATFORM(COCOA)
>diff --git a/Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm b/Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm
>index e644abef126..69972f017e2 100644
>--- a/Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm
>+++ b/Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm
>@@ -30,6 +30,7 @@
> 
> #include <JavaScriptCore/GCActivityCallback.h>
> #include <JavaScriptCore/Heap.h>
>+#include <JavaScriptCore/SamplingProfiler.h>
> #include <JavaScriptCore/VM.h>
> #include <mach/mach.h>
> #include <mach/vm_statistics.h>
>@@ -139,7 +140,7 @@ std::array<TagInfo, 256> pagesPerVMTag()
>     return tags;
> }
> 
>-static Vector<MachSendRight> threadSendRights()
>+static Vector<MachSendRight> filterThreads(Function<bool (mach_port_t)>&& filterFunction)
> {
>     thread_array_t threadList = nullptr;
>     mach_msg_type_number_t threadCount = 0;
>@@ -148,8 +149,10 @@ static Vector<MachSendRight> threadSendRights()
>         return { };
> 
>     Vector<MachSendRight> machThreads;
>-    for (mach_msg_type_number_t i = 0; i < threadCount; ++i)
>-        machThreads.append(MachSendRight::adopt(threadList[i]));
>+    for (mach_msg_type_number_t i = 0; i < threadCount; ++i) {
>+        if (filterFunction(threadList[i]))
>+            machThreads.append(MachSendRight::adopt(threadList[i]));
>+    }
> 
>     kr = vm_deallocate(mach_task_self(), (vm_offset_t)threadList, threadCount * sizeof(thread_t));
>     ASSERT(kr == KERN_SUCCESS);
>@@ -157,10 +160,26 @@ static Vector<MachSendRight> threadSendRights()
>     return machThreads;
> }
> 
>-static float cpuUsage()
>+static Vector<MachSendRight> threadSendRights()
> {
>-    auto machThreads = threadSendRights();
>+    return filterThreads([] (mach_port_t) {
>+        return true;
>+    });
>+}
> 
>+static Vector<MachSendRight> threadSendRightsExcludingDebuggerThreads(mach_port_t resourceUsageThread, mach_port_t samplingProfilerThread)
>+{
>+    return filterThreads([&] (mach_port_t thread) {
>+        if (thread == resourceUsageThread)
>+            return false;
>+        if (thread == samplingProfilerThread)
>+            return false;
>+        return true;
>+    });
>+}
>+
>+static float cpuUsage(Vector<MachSendRight> machThreads)
>+{
>     float usage = 0;
> 
>     for (auto& machThread : machThreads) {
>@@ -205,9 +224,26 @@ static unsigned categoryForVMTag(unsigned tag)
>     }
> }
> 
>+#if ENABLE(SAMPLING_PROFILER)
>+void ResourceUsageThread::platformSaveStateBeforeStarting()
>+{
>+    m_samplingProfilerMachThread = m_vm->samplingProfiler() ? m_vm->samplingProfiler()->machThread() : MACH_PORT_NULL;
>+}
>+#else
>+void ResourceUsageThread::platformSaveStateBeforeStarting() { }
>+#endif
>+
> void ResourceUsageThread::platformCollectCPUData(JSC::VM*, ResourceUsageData& data)
> {
>-    data.cpu = cpuUsage();
>+    data.cpu = cpuUsage(threadSendRights());
>+
>+    mach_port_t resourceUsageThread = mach_thread_self();
>+#if ENABLE(SAMPLING_PROFILER)
>+    mach_port_t samplingProfilerThread = m_samplingProfilerMachThread;
>+#else
>+    mach_port_t samplingProfilerThread = MACH_PORT_NULL;
>+#endif
>+    data.cpuExcludingDebuggerThreads = cpuUsage(threadSendRightsExcludingDebuggerThreads(resourceUsageThread, samplingProfilerThread));
> }
> 
> void ResourceUsageThread::platformCollectMemoryData(JSC::VM* vm, ResourceUsageData& data)
>diff --git a/Source/WebCore/page/linux/ResourceUsageThreadLinux.cpp b/Source/WebCore/page/linux/ResourceUsageThreadLinux.cpp
>index f76f15aa572..3149ff87b69 100644
>--- a/Source/WebCore/page/linux/ResourceUsageThreadLinux.cpp
>+++ b/Source/WebCore/page/linux/ResourceUsageThreadLinux.cpp
>@@ -150,9 +150,17 @@ static float cpuUsage()
>     return clampTo<float>(usage, 0, 100);
> }
> 
>+void ResourceUsageThread::platformSaveStateBeforeStarting()
>+{
>+}
>+
> void ResourceUsageThread::platformCollectCPUData(JSC::VM*, ResourceUsageData& data)
> {
>     data.cpu = cpuUsage();
>+
>+    // FIXME: Exclude the ResourceUsage thread.
>+    // FIXME: Exclude the SamplingProfiler thread.
>+    data.cpuExcludingDebuggerThreads = data.cpu;
> }
> 
> void ResourceUsageThread::platformCollectMemoryData(JSC::VM* vm, ResourceUsageData& data)

Actions: View | Formatted Diff | Diff

Attachments on bug 193796: 360048 | 360058 | 360077 | 360078 | 360156 | 360176