Bug 54836 – Get rid of posixThread in MachineStackMarker::Thread

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Patrick R. Gansterer]

Get rid of posixThread in MachineStackMarker::Thread

[Patrick R. Gansterer]

Created attachment 83105 [details]
Patch

[Alexey Proskuryakov]

Did you verify that this doesn't re-break what <http://trac.webkit.org/changeset/35440> fixed?

[Patrick R. Gansterer]

(In reply to comment #2)
> Did you verify that this doesn't re-break what <http://trac.webkit.org/changeset/35440> fixed?

What's the relation between r35440 and my patch? r35440 changed the type of m_currentThreadRegistrar from WTF::ThreadSpecific<OwnPtr<ThreadRegistrar> > to pthread_key_t. I didn't touched the pthread_getspecific and pthread_setspecific calls.

[Alexey Proskuryakov]

You are right, I got confused looking at svn blame.

There obviously was some reason to have member variables with both platform and POSIX thread identifiers - will need to find out what it was...

[Patrick R. Gansterer]

(In reply to comment #4)
> You are right, I got confused looking at svn blame.
> 
> There obviously was some reason to have member variables with both platform and POSIX thread identifiers - will need to find out what it was...

It was added as 
pthread_t posixThread;
mach_port_t machThread;
in r8067 <http://trac.webkit.org/changeset/8067/trunk/JavaScriptCore/kjs/collector.cpp>.

AFAICS this is only very very old code, which never received cleanup.

[Adam Roben (:aroben)]

Comment on attachment 83105 [details]
Patch

I don't see anything obviously wrong with this patch, but I don't feel familiar enough with the code to r+ it.

[Alexey Proskuryakov]

I'll take a close look soon.

[Alexey Proskuryakov]

Comment on attachment 83105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83105&action=review

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:100
> -        : posixThread(pthread)
> -        , platformThread(platThread)
> +    Thread(const PlatformThread& platThread, void* base)
> +        : platformThread(platThread)

One subtle thing about moving from pthreads to PlatformThreads could be whether the IDs are reused after thread termination. I didn't think much about this, because I have bigger concerns below.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:145
> +    return GetCurrentThread();

MSDN says that GetCurrentThread() returns a special constant that you can pass to threading functions. Does this mean that all PlatformThreads will be numerically the same on Windows?

If this is indeed a mistake, I'm concerned about our regression test coverage.

> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:187
> -    if (pthread_equal(currentPosixThread, m_registeredThreads->posixThread)) {
> +    if (m_registeredThreads->platformThread == currentPlatformThread) {

Is it OK to compare handles and Mach ports numerically? Is that documented somewhere?

[Alexey Proskuryakov]

> AFAICS this is only very very old code, which never received cleanup.

Maciej might remember why he added both pthread_t and mach_port_t member variables. Both were added in the same patch, so there was probably a reason.

[Alexey Proskuryakov]

Maybe because Mach API was needed to suspend threads? See also: bug 26838.

[Eric Seidel (no email)]

Comment on attachment 83105 [details]
Patch

Did we come to any conclusions here?  This looks reasonable to me.  If it doesn't break any tests seems it's worth doing... Or is this sort of thing not testable/tested?

[Alexey Proskuryakov]

Comment on attachment 83105 [details]
Patch

There were no answers to my questions yet.

[Patrick R. Gansterer]

Comment on attachment 83105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83105&action=review

>> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:100
>> +        : platformThread(platThread)
> 
> One subtle thing about moving from pthreads to PlatformThreads could be whether the IDs are reused after thread termination. I didn't think much about this, because I have bigger concerns below.

it's possible that a pthread_t can be reused too (at least on windows) see http://sourceware.org/cgi-bin/cvsweb.cgi/pthreads/ptw32_reuse.c?rev=1.11&cvsroot=pthreads-win32

>> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:145
>> +    return GetCurrentThread();
> 
> MSDN says that GetCurrentThread() returns a special constant that you can pass to threading functions. Does this mean that all PlatformThreads will be numerically the same on Windows?
> 
> If this is indeed a mistake, I'm concerned about our regression test coverage.

http://msdn.microsoft.com/en-us/library/ms683182%28v=vs.85%29.aspx says The return value is a pseudo handle for the current thread.
It behaves "like" GetCurrentThreadId() but uses HANDLE instead of DWORD as return value.

>> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:187
>> +    if (m_registeredThreads->platformThread == currentPlatformThread) {
> 
> Is it OK to compare handles and Mach ports numerically? Is that documented somewhere?

any good idea where i can find this info?

[Patrick R. Gansterer]

Comment on attachment 83105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83105&action=review

>>> Source/JavaScriptCore/runtime/MachineStackMarker.cpp:187
>>> +    if (m_registeredThreads->platformThread == currentPlatformThread) {
>> 
>> Is it OK to compare handles and Mach ports numerically? Is that documented somewhere?
> 
> any good idea where i can find this info?

If you have a look at http://www.opensource.apple.com/source/Libc/Libc-391/pthreads/pthread.c thread_t is compared with a simple "==" too. So this looks ok to me.

[Alexey Proskuryakov]

> If you have a look at http://www.opensource.apple.com/source/Libc/Libc-391/pthreads/pthread.c thread_t is compared with a simple "==" too. So this looks ok to me.

I'm still not sure why not just use pthread_equal.

> http://msdn.microsoft.com/en-us/library/ms683182%28v=vs.85%29.aspx says The return value is a 
> pseudo handle for the current thread.
> It behaves "like" GetCurrentThreadId() but uses HANDLE instead of DWORD as return value.

What do numeric values returned from it look like in practice? With MSDN saying that it's a constant, I don't know how we can use it for unique identifier.

[Patrick R. Gansterer]

(In reply to comment #15)
> > If you have a look at http://www.opensource.apple.com/source/Libc/Libc-391/pthreads/pthread.c thread_t is compared with a simple "==" too. So this looks ok to me.
> 
> I'm still not sure why not just use pthread_equal.

To remove the pthread dependency. There is no working pthread implementation for WinCE ARM and WinCE is broken because of this dependecy at the moment.

> > http://msdn.microsoft.com/en-us/library/ms683182%28v=vs.85%29.aspx says The return value is a 
> > pseudo handle for the current thread.
> > It behaves "like" GetCurrentThreadId() but uses HANDLE instead of DWORD as return value.
> 
> What do numeric values returned from it look like in practice? With MSDN saying that it's a constant, I don't know how we can use it for unique identifier.

5 calls to GetCurrentThreadId() (from different threads; the same thread returns the same value again): 0x00002514, 0x00000940, 0x00002504, 0x00001b1c, 0x000028f4.

[Alexey Proskuryakov]

Thanks!

I don't have any objections to this patch. It should probably be reviewed by JSC experts (CC'ed some of them). I'm also sending an e-mail to webkit-dev to try and figure out what our policy about pthreads should be in general.

[Geoffrey Garen]

> I don't have any objections to this patch. It should probably be reviewed by JSC experts (CC'ed some of them).

Seems OK to me.

[Patrick R. Gansterer]

PING!
Can someone review the patch?

[Geoffrey Garen]

Comment on attachment 83105 [details]
Patch

We know that comparing thread identifiers using == works on Darwin and Windows. However, on USE(PTHREADS) platforms that are not Darwin or Windows, it's technically a violation of the pthreads API to use == instead of pthread_equal.

I'd like to see a follow-up patch that turns PlatformThread into a wrapper class that defines its own operator==, which calls pthread_equal on USE(PTHREADS) platforms.

[WebKit Review Bot]

Comment on attachment 83105 [details]
Patch

Rejecting attachment 83105 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
unk #1 succeeded at 1 with fuzz 3.
patching file Source/JavaScriptCore/runtime/MachineStackMarker.cpp
Hunk #1 FAILED at 96.
Hunk #2 FAILED at 144.
Hunk #3 FAILED at 166.
Hunk #4 FAILED at 182.
Hunk #5 FAILED at 194.
Hunk #6 FAILED at 393.
Hunk #7 FAILED at 405.
7 out of 7 hunks FAILED -- saving rejects to file Source/JavaScriptCore/runtime/MachineStackMarker.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Geoffrey Garen', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/9958134

[Patrick R. Gansterer]

Created attachment 109758 [details]
Patch

Rebased and fixed pthread_equal()

[Patrick R. Gansterer]

Created attachment 109761 [details]
Patch

[Patrick R. Gansterer]

Created attachment 109762 [details]
Patch

[WebKit Review Bot]

Comment on attachment 109762 [details]
Patch

Clearing flags on attachment: 109762

Committed r96722: <http://trac.webkit.org/changeset/96722>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.