Bug 154017 – JSC SamplingProfiler: Use a thread + sleep loop instead of WTF::WorkQueue for taking samples

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

This is a cleaner design.
We will also take better control of keeping consistent sample times closer to 1ms.

[Saam Barati]

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.

[Saam Barati]

Created attachment 270911 [details]
patch

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

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.

[Saam Barati]

Comment on attachment 270911 [details]
patch

I'm going to rebase a modern version of this patch.

[Saam Barati]

Created attachment 275672 [details]
patch

[Geoffrey Garen]

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.

[Saam Barati]

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)"

[Saam Barati]

Created attachment 275738 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 275738 [details]
patch for landing

Clearing flags on attachment: 275738

Committed r199092: <http://trac.webkit.org/changeset/199092>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.