Bug 154017 - JSC SamplingProfiler: Use a thread + sleep loop instead of WTF::WorkQueue for taking samples
Summary: JSC SamplingProfiler: Use a thread + sleep loop instead of WTF::WorkQueue for...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-08 16:21 PST by Saam Barati
Modified: 2016-04-05 20:55 PDT (History)
12 users (show)

See Also:


Attachments
patch (18.33 KB, patch)
2016-02-08 21:49 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (18.12 KB, patch)
2016-04-05 09:58 PDT, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff
patch for landing (18.31 KB, patch)
2016-04-05 19:20 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.