Created attachment 100906 [details] Patch

Comment on attachment 100906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100906&action=review > Source/JavaScriptCore/ChangeLog:9 > + With this change, currentThread is only 5% slower in debug and 10% in release mode. 5% slower and 10% slower than what?

(In reply to comment #2) > (From update of attachment 100906 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100906&action=review > > > Source/JavaScriptCore/ChangeLog:9 > > + With this change, currentThread is only 5% slower in debug and 10% in release mode. > > 5% slower and 10% slower than what? Good point. I really butchered that line. I'll fix it before I check in. It should say "With this change, currentThread is 10% faster than isMainThread in release mode and only %5 slower than isMainThread in debug."

Strictly (paranoidly?) speaking, the code before the change allowed calling WTF::currentThread() before WTF::initializeThreading(). After change, the code assumes initializeThreading is always called first. Could you add an ASSERT to verify the assumption? It could be something like this: pthread_key_t ThreadIdentifierData::m_key = PTHREAD_KEYS_MAX; ... ThreadIdentifier ThreadIdentifierData::identifier() { ASSERT(m_key != PTHREAD_KEYS_MAX); ...

Landed (and fixed a header include issue): http://trac.webkit.org/changeset/91082 http://trac.webkit.org/changeset/91087 http://trac.webkit.org/changeset/91089 And rolled out due to gtk run time issues: http://trac.webkit.org/changeset/91094 Working on gtk fix before landing.

Landed again after the gtk fix. http://trac.webkit.org/changeset/91380

It appears to me that this changeset caused assertion failure in Chromium OS and Chromium Mac (possibility others). Here's a stack trace on Chromium OS: http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28ChromiumOS%20dbg%29%282%29/builds/2774/steps/browser_tests/logs/stdio ASSERTION FAILED: m_key != PTHREAD_KEYS_MAX third_party/WebKit/Source/JavaScriptCore/wtf/ThreadIdentifierDataPthreads.cpp(60) : static WTF::ThreadIdentifier WTF::ThreadIdentifierData::identifier() [7759:7768:0720/203836:986988360721:FATAL:utility_process_host.cc(39)] Check failed: !is_batch_mode_. Backtrace: base::debug::StackTrace::StackTrace() [0x9174f10] logging::LogMessage::~LogMessage() [0x9192f4d] UtilityProcessHost::~UtilityProcessHost() [0xb237a55] ChildProcessHost::OnChildDied() [0x9e77666] BrowserChildProcessHost::OnChildDied() [0xb104192] ChildProcessHost::ListenerHook::OnChannelError() [0x9e77a02] IPC::Channel::ChannelImpl::ClosePipeOnError() [0x9f7197e] IPC::Channel::ChannelImpl::OnFileCanReadWithoutBlocking() [0x9f71665] base::MessagePumpLibevent::FileDescriptorWatcher::OnFileCanReadWithoutBlocking() [0x9161ea8] base::MessagePumpLibevent::OnLibeventNotification() [0x9163cda] event_process_active [0x9829302] event_base_loop [0x9829647] base::MessagePumpLibevent::Run() [0x9163502] MessageLoop::RunInternal() [0x9197afd] MessageLoop::RunHandler() [0x91979d7] MessageLoop::Run() [0x919742b] base::Thread::Run() [0x91d77fb] base::Thread::ThreadMain() [0x91d79cb] base::(anonymous namespace)::ThreadFunc() [0x91d6d62] start_thread [0xf6c7196e] 0xf689bb5e [7759:7768:0720/203836:986988360721:FATAL:utility_process_host.cc(39)] Check failed: !is_batch_mode_. Backtrace: base::debug::StackTrace::StackTrace() [0x9174f10] logging::LogMessage::~LogMessage() [0x9192f4d] UtilityProcessHost::~UtilityProcessHost() [0xb237a55] ChildProcessHost::OnChildDied() [0x9e77666] BrowserChildProcessHost::OnChildDied() [0xb104192] ChildProcessHost::ListenerHook::OnChannelError() [0x9e77a02] IPC::Channel::ChannelImpl::ClosePipeOnError() [0x9f7197e] IPC::Channel::ChannelImpl::OnFileCanReadWithoutBlocking() [0x9f71665] base::MessagePumpLibevent::FileDescriptorWatcher::OnFileCanReadWithoutBlocking() [0x9161ea8] base::MessagePumpLibevent::OnLibeventNotification() [0x9163cda] event_process_active [0x9829302] event_base_loop [0x9829647] base::MessagePumpLibevent::Run() [0x9163502] MessageLoop::RunInternal() [0x9197afd] MessageLoop::RunHandler() [0x91979d7] MessageLoop::Run() [0x919742b] base::Thread::Run() [0x91d77fb] base::Thread::ThreadMain() [0x91d79cb] base::(anonymous namespace)::ThreadFunc() [0x91d6d62] start_thread [0xf6c7196e] 0xf689bb5e

Chromium Mac failures: http://build.chromium.org/p/chromium/builders/Mac%2010.6%20Tests%20%28dbg%29%284%29/builds/2134 http://build.chromium.org/p/chromium/builders/Mac%2010.6%20Tests%20%28dbg%29%282%29/builds/11687 http://build.chromium.org/p/chromium/builders/Mac%2010.5%20Tests%20%28dbg%29%283%29/builds/10242 http://build.chromium.org/p/chromium/builders/Mac%2010.6%20Tests%20%28dbg%29%281%29/builds/13178 http://build.chromium.org/p/chromium/builders/Mac%2010.6%20Tests%20%28dbg%29%282%29/builds/11687 http://build.chromium.org/p/chromium/builders/Mac%2010.6%20Tests%20%28dbg%29%283%29/builds/11138

Chromium Linux: http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28dbg%29%281%29/builds/9741 http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28dbg%29%28shared%29/builds/1463 http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28Views%20dbg%29%281%29/builds/3229 http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28Views%20dbg%29%282%29/builds/3185 http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28Views%20dbg%29%283%29/builds/3347 Chromium OS: http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28ChromiumOS%20dbg%29%282%29/builds/2774 http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28ChromiumOS%20dbg%29%283%29/builds/2776

Rolled out again :(. Now due to bad behavior in Chromium's IndexedDB tests.

(In reply to comment #10) > Rolled out again :(. > > Now due to bad behavior in Chromium's IndexedDB tests. Could it be that there's a bug in Chromium's IndexedDB implementation?

(In reply to comment #11) > (In reply to comment #10) > > Rolled out again :(. > > > > Now due to bad behavior in Chromium's IndexedDB tests. > > Could it be that there's a bug in Chromium's IndexedDB implementation? It suspect the tests (and with the rollout, the behavior is ok but unfortunate), but it could be the IndexedDB implementation. Something results in WTF::currentThread being called before WTF::initializeThreading (which only needs to be called once in the whole process -- of course the db stuff is in a different process in chromium I think).

Committed as http://trac.webkit.org/changeset/92154 (Now that I fixed Chromium downstream.)