Bug 26838 - Multithread support for JSC on UNIX
Summary: Multithread support for JSC on UNIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Major
Assignee: Nobody
: 55378 (view as bug list)
Depends on: 55433
  Show dependency treegraph
Reported: 2009-06-30 06:15 PDT by Martin Zoubek
Modified: 2011-07-20 12:05 PDT (History)
14 users (show)

See Also:

WebKit-r45357-Collector-multithread-support-for-UNIX.diff (5.96 KB, patch)
2009-06-30 06:16 PDT, Martin Zoubek
no flags Details | Formatted Diff | Diff
WebKit-r45374-Collector-multithread-support-for-UNIX.diff (6.68 KB, patch)
2009-07-01 00:27 PDT, Martin Zoubek
mjs: review-
Details | Formatted Diff | Diff
WebKit-r46156-Collector-multithread-support-for-UNIX.diff (6.52 KB, patch)
2009-07-21 05:47 PDT, Martin Zoubek
eric: review-
Details | Formatted Diff | Diff
Proposed patch (6.58 KB, patch)
2011-02-28 13:55 PST, Alejandro G. Castro
mrobinson: review-
Details | Formatted Diff | Diff
Proposed patch (6.59 KB, patch)
2011-02-28 17:00 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Zoubek 2009-06-30 06:15:51 PDT
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.
Comment 1 Martin Zoubek 2009-06-30 06:16:29 PDT
Created attachment 32061 [details]
Comment 2 Jan Alonzo 2009-06-30 06:38:25 PDT
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.
Comment 3 Martin Zoubek 2009-07-01 00:27:58 PDT
Created attachment 32117 [details]
Comment 4 Maciej Stachowiak 2009-07-03 16:52:43 PDT
Comment on attachment 32117 [details]

Some style issues:


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.
Comment 5 Martin Zoubek 2009-07-21 05:47:30 PDT
Created attachment 33173 [details]

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.
Comment 6 Eric Seidel (no email) 2009-08-07 12:34:43 PDT
Comment on attachment 33173 [details]

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.
Comment 7 Jaroslav Franek 2009-08-10 02:39:40 PDT
(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.
Comment 8 Alejandro G. Castro 2011-02-28 13:55:55 PST
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.
Comment 9 Alejandro G. Castro 2011-02-28 13:56:56 PST
*** Bug 55378 has been marked as a duplicate of this bug. ***
Comment 10 WebKit Review Bot 2011-02-28 13:58:49 PST
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.
Comment 11 Martin Robinson 2011-02-28 14:32:35 PST
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
> +        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
> +    // e.g. on FreeBSD 5.4, neundorf@kde.org
> +    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

I don't feel like I know enough about this code to okay this change, but it seems sane to me.
Comment 12 Martin Robinson 2011-02-28 14:34:23 PST
I think I'd also like to see Sunspider before and after results from ToT.
Comment 13 Xan Lopez 2011-02-28 15:16:31 PST
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.
Comment 14 Alexey Proskuryakov 2011-02-28 15:33:23 PST
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).
Comment 15 Alejandro G. Castro 2011-02-28 17:00:28 PST
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 :).
Comment 16 Martin Robinson 2011-02-28 17:07:22 PST
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

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
Comment 17 Alejandro G. Castro 2011-02-28 17:46:09 PST
Added the fixes and landed: http://trac.webkit.org/changeset/79952
Comment 18 WebKit Review Bot 2011-02-28 18:03:47 PST
http://trac.webkit.org/changeset/79952 might have broken SnowLeopard Intel Release (Build)
Comment 19 Mark Rowe (bdash) 2011-02-28 18:06:35 PST
This appears to have broken the Mac builds.
Comment 20 James Robinson 2011-02-28 18:06:47 PST
This broke the snow leopard compile.  Starting a rollout.
Comment 21 Gavin Barraclough 2011-07-20 12:05:01 PDT
From visual inspection, this code was not rolled out, fix still seems to be in ToT.
Restoring resolved/fixed status.