On Fri, Dec 11, 2009 at 4:08 PM, Dmitry Titov <[email protected]> wrote:
> Thanks for sharing your thoughts! > > On Fri, Dec 11, 2009 at 2:44 AM, Jeremy Orlow <[email protected]> wrote: > >> On Thu, Dec 10, 2009 at 6:48 PM, Dmitry Titov <[email protected]>wrote: >> >>> Hi webkit-dev! >>> >>> Trying to add ASSERTS to RefCounted objects ( >>> https://bugs.webkit.org/show_bug.cgi?id=31639) I've added some more >>> calls to WTF::currentThread() and started to get a lot of assertions here: >>> >>> static ThreadIdentifier establishIdentifierForPthreadHandle(pthread_t& >>> pthreadHandle) >>> { >>> ASSERT(!identifierByPthreadHandle(pthreadHandle)); >>> >>> Here is what happens: A thread that is not created by WTF::CreateThread >>> is calling currentThread() in hope to get a ThreadIdentifier. It doesn't >>> have it so we create it and put into ThreadMap. Normally, WTF threads call >>> WTF::detachThread() that removes the ThreadIdentifier from the map. The >>> 'other' threads do not do so - so the TheradIdentifier remains in the map. >>> >>> On OSX, pthreads reuse system handles when one thread terminates and >>> another starts... That easily leads to threads that run sequentially to have >>> the same handle, and the same ThreadIdentifier - since it is still in the >>> threadMap, which makes currentThread() kind of useless since it returns the >>> same id for different threads, the very thing the threadMap is tryign to >>> avoid. >>> >>> I'm thinking about changing the implementation to stop "auto-register" >>> non-WTF threads. >>> >> >> Wouldn't this break binary compatibility? >> > > If you mean nightly builds, OSX Safari runs fine with this. On Windows it's > going to be ok because we use OS-supplied ThreadIdentifiers there. > WebKit is a framework on mac....so I was wondering if any app that depends on the framework will work OK. I have no idea, but my guess is this would be an issue. > Does pthreads have any facility to register a callback that's called on >> thread shutdown? Could we use some sort of thread local storage to keep >> track of whether that thread has been auto-registered yet? >> > > I think I see what you mean. I tried to figure out if it is possible. There > is a callback invoked on destruction of a thread-specific slot... Perhaps it > can be used to un-register the thread if we add a thread-specific slot to > all incoming threads. The problem with those is that they are called in > unspecified order so another thread-specific destructor could re-register a > thread again, by calling currentThread(). Perhaps there is a way to do this > cleanly, it escapes me at the moment... > What if the thread local storage kept track of the threads identifier? If it's empty, then we need to auto-register. If it's there, then we use that. I think that would avoid the race you mentioned. > Maybe all non-registered threads could just use a default thread identifier >> and some sort of warning message could be logged the first time this >> happens? >> > > Most usages of currentThread() are like this: > ASSERT(m_myThreadId == currentThread()); > Basically, they verify that the object is called on the same thread as > created... Returning same identifier for different threads satisfies this > check even if the treads are different. If currentThread() returned the > different identifier each time in case of un-registered thread, it would > force the developer to either use WTF::createThread or WTF::registerThread. > If all non-WTF threads used the same identifier, then all blah == currentThread() asserts would work. Some of the less common blah != currentThread() asserts could break if the current thread and the one it's comparing to are both non-WTF. We could probably find ways around all of those asserts. Instead, lets add a new function, WTF::registerThread() that would >>> establish the ThreadIdentifier for a thread not created by WTF::createThread >>> - and assert in currentThread() if the current thread is unknown to WTF. >>> This way, we could find all the cases of such threads and equip them with >>> registerThread()/detachThread() pair that will keep the thread map in a good >>> state. >>> >>> The currentThread() would look like this: >>> >>> ThreadIdentifier currentThread() >>> { >>> pthread_t currentThread = pthread_self(); >>> if (ThreadIdentifier id = identifierByPthreadHandle(currentThread)) >>> return id; >>> >>> ASSERT_NOT_REACHED(); >>> >>> // Either the thread is not created by WTF::CreateThread() or >>> registered by WTF::registerThread(), or we are getting >>> // a call from thread-specific destructor after WTF::detachThread() >>> was called and ThreadIdentifier removed. >>> // Neither scenario permits reliable thread id tracking, so we can >>> not return a meaningful ThreadIdentifier here. >>> // Normally ThreadIdentifiers are compared so lets generate a fake >>> one-time ThreadIdentifier to fail comparison, if >>> // it is in fact done. >>> static ThreadIdentifier fakeId = minFakeIdentifierCount; >>> >>> if (fakeId == maxFakeIdentifierCount) >>> fakeId = minFakeIdentifierCount; >>> >>> return fakeId++; >>> } >>> >>> What would you say? Any bad feelings about that? >>> >>> Dmitry >>> >>> _______________________________________________ >>> webkit-dev mailing list >>> [email protected] >>> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev >>> >>> >> >
_______________________________________________ webkit-dev mailing list [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
