[JSC] Move platformThreadSignal to WTF
Created attachment 305375 [details] Patch
Comment on attachment 305375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305375&action=review > Source/JavaScriptCore/runtime/JSLock.cpp:112 > + m_ownerThread = currentThread(); I think it does not cause so much cost. Basically, currentThread() just loads a identifier from thread local storage.
Comment on attachment 305375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305375&action=review >> Source/JavaScriptCore/runtime/JSLock.cpp:112 >> + m_ownerThread = currentThread(); > > I think it does not cause so much cost. Basically, currentThread() just loads a identifier from thread local storage. I think this is fine but I would like to run it on some benchmarks first just to make sure. I'll r+ if I don't find any issues.
(In reply to Mark Lam from comment #3) > >> Source/JavaScriptCore/runtime/JSLock.cpp:112 > >> + m_ownerThread = currentThread(); > > > > I think it does not cause so much cost. Basically, currentThread() just loads a identifier from thread local storage. > > I think this is fine but I would like to run it on some benchmarks first > just to make sure. I'll r+ if I don't find any issues. I've run the JSC benchmarks and Speedometer. Perf is neutral.
Comment on attachment 305375 [details] Patch r=me
(In reply to Mark Lam from comment #4) > (In reply to Mark Lam from comment #3) > > >> Source/JavaScriptCore/runtime/JSLock.cpp:112 > > >> + m_ownerThread = currentThread(); > > > > > > I think it does not cause so much cost. Basically, currentThread() just loads a identifier from thread local storage. > > > > I think this is fine but I would like to run it on some benchmarks first > > just to make sure. I'll r+ if I don't find any issues. > > I've run the JSC benchmarks and Speedometer. Perf is neutral. Thanks!!
Committed r214489: <http://trac.webkit.org/changeset/214489>
Comment on attachment 305375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305375&action=review > Source/JavaScriptCore/runtime/VMTraps.cpp:458 > + signalThread(optionalOwnerThread.value(), SIGUSR1); This function is only defined for USE(PTHREAD), no? Won't that break configurations which aren't USE(PTHREAD) but which build VMTraps.cpp?
(In reply to JF Bastien from comment #8) > Comment on attachment 305375 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=305375&action=review > > > Source/JavaScriptCore/runtime/VMTraps.cpp:458 > > + signalThread(optionalOwnerThread.value(), SIGUSR1); > > This function is only defined for USE(PTHREAD), no? Won't that break > configurations which aren't USE(PTHREAD) but which build VMTraps.cpp? There's a fall back to a polling based VMTraps for platforms that don't support this. Right now, the signaling implementation is only used on an opt-in basis.
Comment on attachment 305375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305375&action=review >>> Source/JavaScriptCore/runtime/VMTraps.cpp:458 >>> + signalThread(optionalOwnerThread.value(), SIGUSR1); >> >> This function is only defined for USE(PTHREAD), no? Won't that break configurations which aren't USE(PTHREAD) but which build VMTraps.cpp? > > There's a fall back to a polling based VMTraps for platforms that don't support this. Right now, the signaling implementation is only used on an opt-in basis. Also USE(PTHREAD) is true for all OS(UNIX), and OS(UNIX) is true for OS(DARWIN), etc.