Bug 170097 – [JSC] Move platformThreadSignal to WTF

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Move platformThreadSignal to WTF

[Yusuke Suzuki]

Created attachment 305375 [details]
Patch

[Yusuke Suzuki]

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.

[Mark Lam]

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.

[Mark Lam]

(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.

[Mark Lam]

Comment on attachment 305375 [details]
Patch

r=me

[Yusuke Suzuki]

(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!!

[Yusuke Suzuki]

Committed r214489: <http://trac.webkit.org/changeset/214489>

[JF Bastien]

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?

[Mark Lam]

(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.

[Mark Lam]

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.