[Webkit-unassigned] [Bug 26838] Multithread support for JSC on UNIX

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 28 14:32:35 PST 2011


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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #84113|review?                     |review-
               Flag|                            |




--- Comment #11 from Martin Robinson <mrobinson at webkit.org>  2011-02-28 14:32:35 PST ---
(From update of attachment 84113)
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, neundorf at 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
> +#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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list