Bug 26838 – Multithread support for JSC on UNIX

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Martin Zoubek]

Build with ENABLE_JSC_MULTIPLE_THREADS=1 is currently not supported on UNIX (Linux). It fails to compile Collector.cpp, because it reaches #error directives in suspendThread() and resumeThread().
On UNIX systems (with pthread library present), there are generally no pthread_suspend() and pthread_resume() functions. I have implemented them using the per-thread signal handler trick -
- when a thread is created, its signal mask is changed to accept SIGUSR2 (can be changed to arbitrary maskable signal)
- global signal handler for SIGUSR2 is added
- suspending a thread is realized sending SIGUSR2 to given thread
- signal handler is activated, it calls sigwait to wait for another SIGUSR2
- thread is then resumed by sending SIGUSR2 to that thread

Attached patch implements these changes to Collector.cpp. I have successfully built it on RHEL3 and tested with valgrind and helgrind, running multiple scripts in parallel threads seems to work flawlessly.

[Martin Zoubek]

Created attachment 32061 [details]
WebKit-r45357-Collector-multithread-support-for-UNIX.diff

[Jan Alonzo]

Hi Martin

Thanks for the patch. This needs a ChangeLog and review flag set to r?. See http://webkit.org/coding/contributing.html for more info.

[Martin Zoubek]

Created attachment 32117 [details]
WebKit-r45374-Collector-multithread-support-for-UNIX.diff

[Maciej Stachowiak]

Comment on attachment 32117 [details]
WebKit-r45374-Collector-multithread-support-for-UNIX.diff

Some style issues:

>#define SIG_THREAD_SUSPEND_RESUME SIGUSR2

I'd consider making this a static const int, rather than a #define. We prefer to avoid preprocessor macros.


> static void pth_sig_suspend_resume(int signo)

This function should use WebKit naming style, for example sendSuspendResumeSignal()


> -  markConservatively(static_cast<void*>(&regs), static_cast<void*>(reinterpret_cast<char*>(&regs) + regSize));
> +  if (regSize > 0)
> +              markConservatively(static_cast<void*>(&regs), static_cast<void*>(reinterpret_cast<char*>(&regs) + regSize));

This change seems unrelated to the rest of the patch. Please submit as a separate patch or document the purpose of this change in the ChangeLog.


Also, since changes to this area can be performance-sensitive, please provide before and after SunSpider results (as determined by the run-sunspider script on the command line).

This patch looks great! r- for now to attend to the above issues, and please resubmit with the requested revisions.

[Martin Zoubek]

Created attachment 33173 [details]
WebKit-r46156-Collector-multithread-support-for-UNIX.diff

Hi Maciej,
thanks for your review. Changes since last version of the patch:
- SIG_THREAD_SUSPEND_RESUME becomes static const int
- pth_sig_suspend_resume was renamed
- unrelated check for registers size in Heap::markOtherThreadConservatively() was removed, it is not needed
- signal handler was modified to use sigsuspend() instead of sigwait(), because sigwait() is not async signal safe

Sunspider results:
1. Single threaded gtk+ build
before: 525.5ms +/- 0.4%
after: 524.8ms +/- 0.6% (no change)
2. Multi threaded gtk+ build
before: not buildable
after: 544.3ms +/- 0.7%

Tests were performed on Fedora 11 x86_64 in VMware machine.

[Eric Seidel (no email)]

Comment on attachment 33173 [details]
WebKit-r46156-Collector-multithread-support-for-UNIX.diff

We really really need to use some sort o Threading abstraction for this stuff.  Someone just posted a QNX patch which has similar #ifdef hacks.  Instead we should have a ThreadingGtk and ThreadingQNX files which know how to get things like stackbase.  Collector could then use those.

I don't think we can/should keep polluting Collector.cpp like this.  If Maciej feels differently, he should feel completely welcome to r+ this patch, I will not stand in your way if one of the more-regular JSC authors approves of this.

[Jaroslav Franek]

(In reply to comment #6)

As a SW Architect, I understand your point, but look at the reality: Different people are contributing pieces for only the architectures they are using. We at Acision have limited resources and will not engage in writing abstraction layer for any platform possible while x86_64 Linux is what we use exclusively. Those are business/pragmatic constraints.

In my opinion it is better to refactor >>existing<< contributions into an abstraction layer, rather than waiting for a perfect solution - it usually takes ages.

Should you provide an abstract interface you speak of, we could adjust our piece accordingly. Otherwise Martin's contribution stands as is.

[Alejandro G. Castro]

Created attachment 84113 [details]
Proposed patch

Patch updated, now we have two files outside Collector handling this code: StackBounds and MachineStackMarker. Added also GTK and WORKERS in Platform.h to use JSC_MULTIPLE_THREADS trying to fix bug 55378. There is a style issue but it is a pthread define.

[Alejandro G. Castro]

*** Bug 55378 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Attachment 84113 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/runtime/MachineStackMarker.cpp:102:  SIG_THREAD_SUSPEND_RESUME is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 84113 [details]
Proposed patch

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

Looks good, but I think it could use another iteration. Eric, with regard to your previous comment: Not having these stubs pretty much leaves Non-Safari Pthread ports in a broken state. I think it's valid to separate the task of filling the stubs and then fixing the style issues with the original file. I think that Alex and I would be quite happy to tackle the cleanup here.

>> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:102
>> +static const int SIG_THREAD_SUSPEND_RESUME = SIGUSR2;
> 
> SIG_THREAD_SUSPEND_RESUME is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

I believe you should make this constant just follow WebKit coding style conventions. g_threadSuspendResumeSignal or something similar.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:109
> +    sigset_t sigs;
> +    sigemptyset(&sigs);
> +    sigaddset(&sigs, SIG_THREAD_SUSPEND_RESUME);
> +    sigsuspend(&sigs);

sigs -> signalSet.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:135
> +#if USE(PTHREADS)
> +        struct sigaction act;
> +        act.sa_handler = pthreadSignalHandlerSuspendResume;
> +        sigemptyset(&act.sa_mask);
> +#ifdef SA_RESTART
> +        act.sa_flags = SA_RESTART;
> +#else
> +        act.sa_flags = 0;
> +#endif
> +        sigaction(SIG_THREAD_SUSPEND_RESUME, &act, 0);
> +
> +        sigset_t mask;
> +        sigemptyset(&mask);
> +        sigaddset(&mask, SIG_THREAD_SUSPEND_RESUME);
> +        pthread_sigmask(SIG_UNBLOCK, &mask, 0);
> +#endif

I'm not sure if USE(PTHREADS) is appropriate ifdef here since it appears that other platforms that already have an implementation for these stubs USE(PTHREADS) as well. You probably want to add  !PLATFORM(DARWIN), etc to this and abstract it into a #define at the top.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:368
> +#if HAVE(PTHREAD_NP_H) || PLATFORM(NETBSD)
> +    // e.g. on FreeBSD 5.4, [e-mail]
> +    pthread_attr_get_np(platformThread, &regs);

I guess configure on NetBSD doesn't declare PTHREAD_NP_H? Is this bug documented somewhere?

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:418
> +    int rc = pthread_attr_getstack(&regs, &stackBase, &stackSize);
> +    (void)rc; // FIXME: Deal with error code somehow? Seems fatal.

Is it really okay here to not check the return value of pthread_attr_getstack? I'd rather see the FIXME resolved before this patch goes in.

> Source/JavaScriptCore/wtf/Platform.h:566
> +#if (ENABLE(WORKERS) || PLATFORM(GTK) || PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || (PLATFORM(QT) && OS(DARWIN) && !ENABLE(SINGLE_THREADED))) && !defined(ENABLE_JSC_MULTIPLE_THREADS)

I don't feel like I know enough about this code to okay this change, but it seems sane to me.

[Martin Robinson]

I think I'd also like to see Sunspider before and after results from ToT.

[Xan Lopez]

Comment on attachment 84113 [details]
Proposed patch

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

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:128
> +#endif

Can't we just define SA_RESTART do 0 when it's not present/available/whatever and be done with the check?

>> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:368
>> +    pthread_attr_get_np(platformThread, &regs);
> 
> I guess configure on NetBSD doesn't declare PTHREAD_NP_H? Is this bug documented somewhere?

I'd say let's get rid of this if we cannot test it... NetBSD people can submit patches.

[Alexey Proskuryakov]

See also discussion in bug 54836 (I'm not thinking of anything in particular, but seems useful to be aware of other work being done in the same area right now).

[Alejandro G. Castro]

Created attachment 84154 [details]
Proposed patch

I tried to keep the code similar to the one we have in StackBounds, seems the best option considering that code is already used. I've removed the WORKERS condition from Platform.h and just left the GTK, it is enough for the moment we take care of the issues in GTK and later we can try to check other situations :).

[Martin Robinson]

Comment on attachment 84154 [details]
Proposed patch

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

Okay, with the few fixes I mentioned. This needs to be cleaned up, but the GTK+ bots are dying in a bloody heap at the moment.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:74
> +#if USE(PTHREADS)

Please use if !OS(WINDOWS) && !OS(DARWIN) && USE(PTHREADS) here as well.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:77
> +#define SA_RESTART 0

Probably should just "#error MachineStackMarker requires SA_RESTART" (or something nicer sounding) here. I don't think 0 is actually a working replacement.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:124
> +        struct sigaction act;

act -> action

[Alejandro G. Castro]

Added the fixes and landed: http://trac.webkit.org/changeset/79952

[WebKit Review Bot]

http://trac.webkit.org/changeset/79952 might have broken SnowLeopard Intel Release (Build)

[Mark Rowe (bdash)]

This appears to have broken the Mac builds.

[James Robinson]

This broke the snow leopard compile.  Starting a rollout.

[Gavin Barraclough]

From visual inspection, this code was not rolled out, fix still seems to be in ToT.
Restoring resolved/fixed status.