Bug 54832 – Remove pthreads dependency in JSLock when it's not needed

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

Use WTF::Mutex in JSLock

[Patrick R. Gansterer]

Created attachment 83101 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 83101 [details]
Patch

There is a race condition initializing the static mutex now. Typically, we would create the static mutex from JSC::initializeThreading().

But I don't think that you need JSLock to work at all. The only times it's used with LockForReal argument is for deprecated Mac OS X SPI behavior where concurrent access to a JS context was serialized with a mutex automagically.

[Patrick R. Gansterer]

(In reply to comment #2)
> (From update of attachment 83101 [details])
> There is a race condition initializing the static mutex now. Typically, we would create the static mutex from JSC::initializeThreading().
> 
> But I don't think that you need JSLock to work at all. The only times it's used with LockForReal argument is for deprecated Mac OS X SPI behavior where concurrent access to a JS context was serialized with a mutex automagically.

Is it safe to remove the Mutex as a whole? Or should i upload a new patch with a initializeThreading() call?

[Alexey Proskuryakov]

I suggest making JSMutex no-op on non-Mac platforms (like it's no-op on Mac when SilenceAssertionsOnly is passed).

[Patrick R. Gansterer]

Created attachment 83466 [details]
Patch

I digged a little bit deeper into this, but AKAICS I don't want to remove it as a whole.
So this patch removes the race condition from the previous patch.

[Alexey Proskuryakov]

So, why don't you want to make JSLock no-op on non-Mac platforms?

What is "AKAICS"?

[Patrick R. Gansterer]

Created attachment 83514 [details]
Patch

[Darin Adler]

Comment on attachment 83514 [details]
Patch

I don’t think USE(PTHREADS) is the correct thing here. The obsolete execution model only matters on certain platforms. It’s arguably a "coincidence" that USE(PTHREADS) happens to be true on those platforms.

[Patrick R. Gansterer]

(In reply to comment #8)
> (From update of attachment 83514 [details])
> I don’t think USE(PTHREADS) is the correct thing here. The obsolete execution model only matters on certain platforms. It’s arguably a "coincidence" that USE(PTHREADS) happens to be true on those platforms.

ap (and me too) preferred USE(PTHREADS) over OS(DARWIN), so you get the ASSERTION on other platforms when using pthread too.

[Alexey Proskuryakov]

it's true that due to this change, we'll subtly break Mac if we switch from pthreads to something else. May be best to add an #error for this case.

#if PLATFORM(MAC) && !USE(PTHREADS)
#error...

[Patrick R. Gansterer]

Created attachment 83530 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 83530 [details]
Patch

OK. I still think that an #error would have been better, because someone may remove the seemingly redundant OS(DARWIN) || USE(PTHREADS) check without deep thinking in the future.

[Patrick R. Gansterer]

Comment on attachment 83530 [details]
Patch

(In reply to comment #12)
> (From update of attachment 83530 [details])
> OK. I still think that an #error would have been better, because someone may remove the seemingly redundant OS(DARWIN) || USE(PTHREADS) check without deep thinking in the future.

I also made a version with #error, but the resulting code looked a kind of ugly to me.
The three lines of comments explaining why we have OS(DARWIN) in the #if will be part of a possible removal patch, so it shouldn't be too easy to do this by accident.

[WebKit Commit Bot]

Comment on attachment 83530 [details]
Patch

Clearing flags on attachment: 83530

Committed r79564: <http://trac.webkit.org/changeset/79564>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.