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

[patch] [PATCH] Proposed Fix

exclude-5.patch (text/plain), 8.76 KB, created by Joseph Pecoraro on 2019-01-25 13:47:30 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Joseph Pecoraro

Created: 2019-01-25 13:47:30 PST

Size: 8.76 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..472aed899e6 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,39 @@
>+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,
>+        we 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 to threading identifying the
>+        SamplingProfiler thread. The SamplingProfiler thread 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..d8e54be9d34 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>
>@@ -157,10 +158,8 @@ static Vector<MachSendRight> threadSendRights()
>     return machThreads;
> }
> 
>-static float cpuUsage()
>+static float cpuUsage(Vector<MachSendRight>& machThreads)
> {
>-    auto machThreads = threadSendRights();
>-
>     float usage = 0;
> 
>     for (auto& machThread : machThreads) {
>@@ -205,9 +204,32 @@ static unsigned categoryForVMTag(unsigned tag)
>     }
> }
> 
>+void ResourceUsageThread::platformSaveStateBeforeStarting()
>+{
>+#if ENABLE(SAMPLING_PROFILER)
>+    m_samplingProfilerMachThread = m_vm->samplingProfiler() ? m_vm->samplingProfiler()->machThread() : MACH_PORT_NULL;
>+#endif
>+}
>+
> void ResourceUsageThread::platformCollectCPUData(JSC::VM*, ResourceUsageData& data)
> {
>-    data.cpu = cpuUsage();
>+    Vector<MachSendRight> threads = threadSendRights();
>+    data.cpu = cpuUsage(threads);
>+
>+    // Remove debugger threads.
>+    mach_port_t resourceUsageMachThread = mach_thread_self();
>+    threads.removeAllMatching([&] (MachSendRight& thread) {
>+        mach_port_t machThread = thread.sendRight();
>+        if (machThread == resourceUsageMachThread)
>+            return true;
>+#if ENABLE(SAMPLING_PROFILER)
>+        if (machThread == m_samplingProfilerMachThread)
>+            return true;
>+#endif
>+        return false;
>+    });
>+
>+    data.cpuExcludingDebuggerThreads = cpuUsage(threads);
> }
> 
> 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