[webkit-reviews] review denied: [Bug 26838] Multithread support for JSC on UNIX : [Attachment 32117] WebKit-r45374-Collector-multithread-support-for-UNIX.diff

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Martin Zoubek
<martin.zoubek at acision.com>'s request for review:
Bug 26838: Multithread support for JSC on UNIX
https://bugs.webkit.org/show_bug.cgi?id=26838

Attachment 32117: WebKit-r45374-Collector-multithread-support-for-UNIX.diff
https://bugs.webkit.org/attachment.cgi?id=32117&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
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.