Get rid of posixThread in MachineStackMarker::Thread
Created attachment 83105 [details] Patch
Did you verify that this doesn't re-break what <http://trac.webkit.org/changeset/35440> fixed?
(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.
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...
(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.
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.
I'll take a close look soon.
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?
> 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.
Maybe because Mach API was needed to suspend threads? See also: bug 26838.
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?
Comment on attachment 83105 [details] Patch There were no answers to my questions yet.
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?
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.
> 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.
(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.
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.
> 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.
PING! Can someone review the patch?
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.
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
Created attachment 109758 [details] Patch Rebased and fixed pthread_equal()
Created attachment 109761 [details] Patch
Created attachment 109762 [details] Patch
Comment on attachment 109762 [details] Patch Clearing flags on attachment: 109762 Committed r96722: <http://trac.webkit.org/changeset/96722>
All reviewed patches have been landed. Closing bug.