Bug 64577 - currentThread is too slow!
Summary: currentThread is too slow!
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on: 64616 64871 64924
Blocks: 31639
  Show dependency treegraph
 
Reported: 2011-07-14 17:47 PDT by David Levin
Modified: 2011-08-01 17:11 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.68 KB, patch)
2011-07-14 17:54 PDT, David Levin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2011-07-14 17:47:24 PDT
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.
Comment 1 David Levin 2011-07-14 17:54:19 PDT
Created attachment 100906 [details]
Patch
Comment 2 Darin Adler 2011-07-14 17:56:22 PDT
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?
Comment 3 David Levin 2011-07-14 18:00:32 PDT
(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."
Comment 4 Dmitry Titov 2011-07-14 18:37:07 PDT
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);
  ...
Comment 5 David Levin 2011-07-20 07:52:08 PDT
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.
Comment 6 David Levin 2011-07-20 11:22:15 PDT
Landed again after the gtk fix.

http://trac.webkit.org/changeset/91380
Comment 7 Ryosuke Niwa 2011-07-20 21:11:20 PDT
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
Comment 10 David Levin 2011-07-20 22:12:10 PDT
Rolled out again :(.

Now due to bad behavior in Chromium's IndexedDB tests.
Comment 11 Ryosuke Niwa 2011-07-20 22:13:21 PDT
(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?
Comment 12 David Levin 2011-07-20 22:18:04 PDT
(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).
Comment 13 David Levin 2011-08-01 17:11:31 PDT
Committed as http://trac.webkit.org/changeset/92154

(Now that I fixed Chromium downstream.)