Bug 153584 – Enable SamplingProfiler on POSIX environment

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Discussed with Saam, and my rough design is as follows.

= Suspend
1. in the JS thread, install sigaction to the JS execution thread (This should be done in the constructor of the MachineThread)
2. in the profiler, emit signal with pthread_kill and wait with POSIX semaphore
3. in the signal handler, up the POSIX semaphore. Use sem_post because it is the async-signal-safe function in the UNIX environment[1].
4. in the signal handler, perform sigsuspend to stop the thread until being resumed.
5. in the profiler, we can be waken up from the semaphore because (3) ups.

[1]: http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html#tag_02_04_03

NOTE:
Be careful to sigaction timing. Should not emit pthread_kill for the thread that does not install signal handlers yet. (But in the current case, it should not be possible).

= Resume
1. in the profiler, emit signal and wait on the semaphore
2. in the signal handler, it is waken up from the sigsuspend
3. in the signal handler, up the semaphore
4. in the profiler, the profiler is waken up from the semaphore. It is ensured that the given thread is resumed by the signal.

[Yusuke Suzuki]

Created attachment 270116 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 270116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270116&action=review

some notes. I'll fix some build failures.

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:-67
> -#if defined(SA_RESTART)

I think this ifdef is not used anymore.
Because Thread::suspend / resume always emits signals. But signal handler is only installed when SA_RESTART is defined.
So in the environment that there is no SA_RESTART, the process will be terminated.

> Source/WTF/wtf/Platform.h:433
> +#endif

Oops, I'll fix this part.

[Yusuke Suzuki]

Comment on attachment 270116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270116&action=review

> Source/JavaScriptCore/heap/MachineStackMarker.h:132
> +        int suspendCount;

Oops, I'll set { 0 }.

[Yusuke Suzuki]

Comment on attachment 270116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270116&action=review

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:377
> +        LockHolder lock(suspendLock);

I'll change this lock to the static global lock.
Consider the following case, there are threads A and B.
And A attempt to suspend B and B attempt to suspend A.
In that case, if A and B can emit pthread_kill, both will be suspended.

[Yusuke Suzuki]

Created attachment 270125 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 270125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270125&action=review

Add notes.

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:74
> +

And we should note that, when signal comes, first, system calls signal handler.
And later, sigsuspend will be resumed. Signal handler invocation always precedes.
http://pubs.opengroup.org/onlinepubs/009695399/functions/sigsuspend.html
"If the action is to execute a signal-catching function, then sigsuspend() shall return after the signal-catching function returns, with the signal mask restored to the set that existed prior to the sigsuspend() call."

So, the probelm never happens that suspended.store(true, ...) will be executed before the handler is called.

Later, I'll insert this note here.

[Yusuke Suzuki]

Comment on attachment 270125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270125&action=review

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:384
> +        LockHolder lock(globalSignalLock);

One approach to avoid global lock is that,

1. each thread has a lock.
2. when suspending / resuming, we acquire (1) the suspended thread's lock and (2) the current thread's lock at the same time. (using std::lock lock(A, B) can avoid dead lock :))

But I think the global lock is enough in the meantime.

[Michael Saboff]

Comment on attachment 270125 [details]
Patch

r=me

It would be good if you could also implement MachineThreads::Thread::Registers::stackPointer() the same way.

[Yusuke Suzuki]

(In reply to comment #8)
> Comment on attachment 270125 [details]
> Patch
> 
> r=me

Thank you for your review!

> 
> It would be good if you could also implement
> MachineThreads::Thread::Registers::stackPointer() the same way.

Yeah! It is nice.

While instructionPointer() and framePointer() are only used for SamplingProfiler (And only built with ENABLE_SAMPLING_PROFILER), stackPointer() is always used to retrieve stack areas for conservative GC.

Currently, only ENABLE_SAMPLING_PROFILER's configuration can retrieve stack pointers. So under non ENABLE_SAMPLING_PROFILER environment, we still need to keep current portable implementation (using pthread). But I think it's not good.

Ideally, we would like to support all the supported environment, make ENABLE_SAMPLING_PROFILER always on and drop current portable implementation completely.

So, I'll discuss what platform (CPU arch, OS, and standard library (BSD LIBC, GLIBC, Android Bionic etc.)) existing ports (GTK and EFL) support.
And at that time, we fill all the platform specific code and implement the above too :)

https://bugs.webkit.org/show_bug.cgi?id=153638

[Yusuke Suzuki]

(In reply to comment #9)
> 
> Yeah! It is nice.
> 
> While instructionPointer() and framePointer() are only used for
> SamplingProfiler (And only built with ENABLE_SAMPLING_PROFILER),
> stackPointer() is always used to retrieve stack areas for conservative GC.
> 
> Currently, only ENABLE_SAMPLING_PROFILER's configuration can retrieve stack
> pointers. So under non ENABLE_SAMPLING_PROFILER environment, we still need
> to keep current portable implementation (using pthread). But I think it's
> not good.
> 
> Ideally, we would like to support all the supported environment, make
> ENABLE_SAMPLING_PROFILER always on and drop current portable implementation
> completely.
> 
> So, I'll discuss what platform (CPU arch, OS, and standard library (BSD
> LIBC, GLIBC, Android Bionic etc.)) existing ports (GTK and EFL) support.
> And at that time, we fill all the platform specific code and implement the
> above too :)
> 
> https://bugs.webkit.org/show_bug.cgi?id=153638

Or, maybe, for fallback, keeping pthread portable implementation may be nice...

[Yusuke Suzuki]

Hmm, the current implementation almost works fine.
But only call-varargs-from-inlined-code-with-odd-number-of-arguments.js with ftl-no-cjit-validate-sampling-profiler mode causes an error. It reports that expected value is different, value becomes NaN.

I'm still not sure why it is caused. But after investigating that,

1. It is caused in DFG phase. During DFG, the register becomes unexpected value.
2. I removed all the logic execpt for pthread_kill. signal handler is empty, and suspend and resume just calls pthread_kill and do nothing more. SamplingProfiler just calls suspend() and resume() and do nothing more. Even in that case, still crash occurs.

[Saam Barati]

Yusuke,
I just landed a patch that adds one more register in http://trac.webkit.org/changeset/195865
The function is called llintPC().
Can you also add it in your patch? It returns whichever register regT4
is on the particular ISA.

[Saam Barati]

(In reply to comment #11)
> Hmm, the current implementation almost works fine.
> But only call-varargs-from-inlined-code-with-odd-number-of-arguments.js with
> ftl-no-cjit-validate-sampling-profiler mode causes an error. It reports that
> expected value is different, value becomes NaN.
> 
> I'm still not sure why it is caused. But after investigating that,
> 
> 1. It is caused in DFG phase. During DFG, the register becomes unexpected
> value.
> 2. I removed all the logic execpt for pthread_kill. signal handler is empty,
> and suspend and resume just calls pthread_kill and do nothing more.
> SamplingProfiler just calls suspend() and resume() and do nothing more. Even
> in that case, still crash occurs.

Is this a recent regression? Does it happen on ToT?
Or does this have to do w/ ::stackPointer()?

[Yusuke Suzuki]

(In reply to comment #12)
> Yusuke,
> I just landed a patch that adds one more register in
> http://trac.webkit.org/changeset/195865
> The function is called llintPC().
> Can you also add it in your patch? It returns whichever register regT4
> is on the particular ISA.

Sure!

[Yusuke Suzuki]

Created attachment 270304 [details]
Patch

Simple test patch that causes DFG issue

[Yusuke Suzuki]

(In reply to comment #13)
> Is this a recent regression? Does it happen on ToT?
> Or does this have to do w/ ::stackPointer()?

Without stackPointer change. And even if we just call signal and do nothing, the issue occurs.

I think, the above patch itself does not have any problems and DFG or elsewhere have some problem...
I uploaded some very very simple patch. That just emit signal in ::suspend and do nothing in ::resume. Signal handler does nothing. And SamplingProfiler just calls suspend and resume periodically.

Even in the above situation, call-varargs-from-inlined-code-with-odd-number-of-arguments.js sometimes fails. So I think the following situation.

1. Signal handler is set with SA_RESTART. But some system calls does not restart. For example, sleep, usleep are the cases in UNIX. We need to fix this anyway (I'll open the bug for that)
2. So, in some place, sleep is interrupted.
3. As a result, the path that is rarely taken may be taken. For example, if you set some threshold time for invoking DFG, it may not be executed in the usual test cases. But due to interrupted sleep, it may be invoked.
4. And since this path has some issue, it causes the test failure, the result becomes NaN.

So I think there are some issues in DFG because when disabling DFG (with env variables), the issue does not occur.

So in the meantime, I'll land the patch with stackPointer and LLInt PC change with masking the above test. And I'll open 2 issues.

1. Making non-restarted syscalls signal-safe. (Like sleep)
2. Tracking call-varargs-from-inlined-code-with-odd-number-of-arguments.js issue

[Yusuke Suzuki]

Committed r195891: <http://trac.webkit.org/changeset/195891>