Bug 154017

Summary: JSC SamplingProfiler: Use a thread + sleep loop instead of WTF::WorkQueue for taking samples
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: 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 Flags
patch
none
patch
ggaren: review+
patch for landing none

Description Saam Barati 2016-02-08 16:21:32 PST
This is a cleaner design.
We will also take better control of keeping consistent sample times closer to 1ms.
Comment 1 Saam Barati 2016-02-08 17:14:12 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.
Comment 2 Saam Barati 2016-02-08 21:49:26 PST
Created attachment 270911 [details]
patch
Comment 3 WebKit Commit Bot 2016-02-08 21:51:39 PST
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 4 Joseph Pecoraro 2016-02-09 11:10:17 PST
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 5 Saam Barati 2016-03-31 16:25:20 PDT
Comment on attachment 270911 [details]
patch

I'm going to rebase a modern version of this patch.
Comment 6 Saam Barati 2016-04-05 09:58:32 PDT
Created attachment 275672 [details]
patch
Comment 7 Geoffrey Garen 2016-04-05 11:08:35 PDT
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 8 Saam Barati 2016-04-05 19:19:51 PDT
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)"
Comment 9 Saam Barati 2016-04-05 19:20:54 PDT
Created attachment 275738 [details]
patch for landing
Comment 10 WebKit Commit Bot 2016-04-05 20:55:14 PDT
Comment on attachment 275738 [details]
patch for landing

Clearing flags on attachment: 275738

Committed r199092: <http://trac.webkit.org/changeset/199092>
Comment 11 WebKit Commit Bot 2016-04-05 20:55:19 PDT
All reviewed patches have been landed.  Closing bug.