Summary: | JSC SamplingProfiler: Use a thread + sleep loop instead of WTF::WorkQueue for taking samples | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, fpizlo, ggaren, gskachkov, joepeck, keith_miller, mark.lam, msaboff, oliver, sukolsak, ysuzuki | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Saam Barati
2016-02-08 16:21:32 PST
We would ask WorkQueue to wake us up in 1ms and regularly be woken up in 1.7ms. We're getting wake up times much closer to what we ask for from just using sleep. There is also less overhead from time when woken up to time when doing work by not using GCD. Created attachment 270911 [details]
patch
Attachment 270911 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:212: This { should be at the end of the previous line [whitespace/braces] [4]
Total errors found: 1 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270911 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=270911&action=review > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:188 > , m_timingInterval(std::chrono::microseconds(1000)) This could be: std::chrono::milliseconds(1) > Source/JavaScriptCore/runtime/VM.cpp:320 > + RefPtr<Stopwatch> stopwatch = Stopwatch::create(); Might be clearer if this is a Ref<> and not a RefPtr<> since there is only this instance that will be moved. Comment on attachment 270911 [details]
patch
I'm going to rebase a modern version of this patch.
Created attachment 275672 [details]
patch
Comment on attachment 275672 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=275672&action=review r=me > Source/JavaScriptCore/ChangeLog:16 > + higher rates, this patch is a performance regression. It's slower because > + we're sampling more frequently. You should state the magnitude here. > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:209 > + ref(); // Matching deref() is timerLoop() when we shut down. > + m_threadIdentifier = createThread(createThreadCallback, this, "jsc.sampling-profiler.thread"); You can avoid manual ref/deref and manual casting, which are anti-patterns, by using the lambda version of createThread: if (m_threadIdentifier) return; RefPtr<SamplingProfiler> profiler = this; createThread("jsc.sampling-profiler.thread", [profiler]() { profiler->timerLoop() }); > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:235 > + std::this_thread::sleep_for(m_timingInterval > stackTraceProcessingTime ? m_timingInterval - stackTraceProcessingTime : m_timingInterval); I think you want m_timingInterval - std::min(m_timingInterval, stackTraceProcessingTime). Otherwise, if sampling takes 0.99ms, you'll delay 0.01ms, but if it takes 1.0ms, you'll delay 1.0ms. Alternatively, you could do 1.0ms unconditionally, based on your argument that execution is paused during (most of) stack processing time. Comment on attachment 275672 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=275672&action=review >> Source/JavaScriptCore/ChangeLog:16 >> + we're sampling more frequently. > > You should state the magnitude here. Will do. >> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:209 >> + m_threadIdentifier = createThread(createThreadCallback, this, "jsc.sampling-profiler.thread"); > > You can avoid manual ref/deref and manual casting, which are anti-patterns, by using the lambda version of createThread: > > if (m_threadIdentifier) > return; > > RefPtr<SamplingProfiler> profiler = this; > createThread("jsc.sampling-profiler.thread", [profiler]() { > profiler->timerLoop() > }); This is much nicer. I didn't know we had this API. I've updated to use it. >> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:235 >> + std::this_thread::sleep_for(m_timingInterval > stackTraceProcessingTime ? m_timingInterval - stackTraceProcessingTime : m_timingInterval); > > I think you want m_timingInterval - std::min(m_timingInterval, stackTraceProcessingTime). > > Otherwise, if sampling takes 0.99ms, you'll delay 0.01ms, but if it takes 1.0ms, you'll delay 1.0ms. > > Alternatively, you could do 1.0ms unconditionally, based on your argument that execution is paused during (most of) stack processing time. I'm going with your "m_timingInterval - std::min(m_timingInterval, stackTraceProcessingTime)" Created attachment 275738 [details]
patch for landing
Comment on attachment 275738 [details] patch for landing Clearing flags on attachment: 275738 Committed r199092: <http://trac.webkit.org/changeset/199092> All reviewed patches have been landed. Closing bug. |