Bug 64577 – currentThread is too slow!

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Levin]

currentThread is much slower than isMainThread.

This leads to weird coding patterns (using isMainThread and that api can't even be used reliably in wtf because wtf may be used prior to initialize the main thread).

In short, the slowness should be fixed.

[David Levin]

Created attachment 100906 [details]
Patch

[Darin Adler]

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?

[David Levin]

(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."

[Dmitry Titov]

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);
  ...

[David Levin]

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.

[David Levin]

Landed again after the gtk fix.

http://trac.webkit.org/changeset/91380

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[David Levin]

Rolled out again :(.

Now due to bad behavior in Chromium's IndexedDB tests.

[Ryosuke Niwa]

(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?

[David Levin]

(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).

[David Levin]

Committed as http://trac.webkit.org/changeset/92154

(Now that I fixed Chromium downstream.)