Thanks for sharing your thoughts!<br><br><div class="gmail_quote">On Fri, Dec 11, 2009 at 2:44 AM, Jeremy Orlow <span dir="ltr">&lt;<a href="mailto:jorlow@chromium.org">jorlow@chromium.org</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">On Thu, Dec 10, 2009 at 6:48 PM, Dmitry Titov <span dir="ltr">&lt;<a href="mailto:dimich@chromium.org" target="_blank">dimich@chromium.org</a>&gt;</span> wrote:<br></div><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Hi webkit-dev!<div><br></div><div>Trying to add ASSERTS to RefCounted objects (<a href="https://bugs.webkit.org/show_bug.cgi?id=31639" target="_blank">https://bugs.webkit.org/show_bug.cgi?id=31639</a>) I&#39;ve added some more calls to WTF::currentThread() and started to get a lot of assertions here:</div>



<div><br></div><div><div><font face="&#39;courier new&#39;, monospace">static ThreadIdentifier establishIdentifierForPthreadHandle(pthread_t&amp; pthreadHandle)</font></div><div><font face="&#39;courier new&#39;, monospace">{</font></div>



<div><font face="&#39;courier new&#39;, monospace">    ASSERT(!identifierByPthreadHandle(pthreadHandle));</font></div><div><br></div><div>Here is what happens: A thread that is not created by WTF::CreateThread is calling currentThread() in hope to get a ThreadIdentifier. It doesn&#39;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 &#39;other&#39; threads do not do so - so the TheradIdentifier remains in the map. </div>



<div><br></div><div>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.</div>



<div><br></div><div>I&#39;m thinking about changing the implementation to stop &quot;auto-register&quot; non-WTF threads.</div></div></blockquote><div><br></div></div><div>Wouldn&#39;t this break binary compatibility?</div>
</div></blockquote><div><br></div><div>If you mean nightly builds, OSX Safari runs fine with this. On Windows it&#39;s going to be ok because we use OS-supplied ThreadIdentifiers there.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="gmail_quote"><div>Does pthreads have any facility to register a callback that&#39;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?</div>
</div></blockquote><div><br></div><div>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...</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="gmail_quote"><div>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?</div>
</div></blockquote><div><br></div><div>Most usages of currentThread() are like this:</div><div><font class="Apple-style-span" face="&#39;courier new&#39;, monospace">ASSERT(m_myThreadId == currentThread());</font></div><div>
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. </div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">

<div><div>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.</div>



<div><br></div><div>The currentThread() would look like this:</div><div><br></div><div><div><font face="&#39;courier new&#39;, monospace">ThreadIdentifier currentThread()</font></div><div><font face="&#39;courier new&#39;, monospace">{</font></div>



<div><font face="&#39;courier new&#39;, monospace">    pthread_t currentThread = pthread_self();</font></div><div><font face="&#39;courier new&#39;, monospace">    if (ThreadIdentifier id = identifierByPthreadHandle(currentThread))</font></div>



<div><font face="&#39;courier new&#39;, monospace">        return id;</font></div><div><font face="&#39;courier new&#39;, monospace"><br></font></div><div><font face="&#39;courier new&#39;, monospace">    ASSERT_NOT_REACHED();</font></div>



<div><font face="&#39;courier new&#39;, monospace"><br></font></div><div><font face="&#39;courier new&#39;, monospace">    // Either the thread is not created by WTF::CreateThread() or registered by WTF::registerThread(), or we are getting</font></div>



<div><font face="&#39;courier new&#39;, monospace">    // a call from thread-specific destructor after WTF::detachThread() was called and ThreadIdentifier removed.</font></div><div><font face="&#39;courier new&#39;, monospace">    // Neither scenario permits reliable thread id tracking, so we can not return a meaningful ThreadIdentifier here.</font></div>



<div><font face="&#39;courier new&#39;, monospace">    // Normally ThreadIdentifiers are compared so lets generate a fake one-time ThreadIdentifier to fail comparison, if</font></div><div><font face="&#39;courier new&#39;, monospace">    // it is in fact done.</font></div>



<div><font face="&#39;courier new&#39;, monospace">    static ThreadIdentifier fakeId = minFakeIdentifierCount;</font></div><div><font face="&#39;courier new&#39;, monospace"><br>
</font></div><div><font face="&#39;courier new&#39;, monospace">    if (fakeId == maxFakeIdentifierCount)</font></div><div><font face="&#39;courier new&#39;, monospace">        fakeId = minFakeIdentifierCount;</font></div>



<div><font face="&#39;courier new&#39;, monospace"><br></font></div><div><font face="&#39;courier new&#39;, monospace">    return fakeId++;</font></div><div><font face="&#39;courier new&#39;, monospace">}</font></div>
<div><br></div><div>What would you say? Any bad feelings about that?</div><div><br></div><font color="#888888"><div>Dmitry</div></font></div></div>
<br></div><div class="im">_______________________________________________<br>
webkit-dev mailing list<br>
<a href="mailto:webkit-dev@lists.webkit.org" target="_blank">webkit-dev@lists.webkit.org</a><br>
<a href="http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev" target="_blank">http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev</a><br>
<br></div></blockquote></div><br>
</blockquote></div><br>