Bug 22364 - Crashes seen on Tiger buildbots due to worker threads exhausting pthread keys
Summary: Crashes seen on Tiger buildbots due to worker threads exhausting pthread keys
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-19 13:01 PST by Alexey Proskuryakov
Modified: 2008-11-20 10:13 PST (History)
0 users

See Also:


Attachments
proposed fix (5.34 KB, patch)
2008-11-20 02:16 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2008-11-19 13:01:11 PST
Currently, each heap creates a pthread key that tracks threads that use this heap. This isn't necessary for workers, which are tied to a single thread.
Comment 1 Alexey Proskuryakov 2008-11-20 02:16:18 PST
Created attachment 25307 [details]
proposed fix
Comment 2 Darin Adler 2008-11-20 09:17:33 PST
Comment on attachment 25307 [details]
proposed fix

>  #ifndef NDEBUG
> +        int error =
>  #endif
> +            pthread_key_delete(m_currentThreadRegistrar);
> +        ASSERT(!error);
> +    }

The little ditties we do so we can assert the results of these functions are ugly enough that I think we should instead, in the future, make little helper inline functions to encapsulate them. They distract too much from the rest of the flow of the functions they are in.

> +void Heap::makeUsableFromMultipleThreads()
> +{
> +    if (m_currentThreadRegistrar)
> +        return;
> +
> +    int error = pthread_key_create(&m_currentThreadRegistrar, unregisterThread);
> +    if (error)
> +        CRASH();
> +}

I wonder what we should do about cases like this. I think I understand what's going on -- this is an assertion, but one so important that you'd like it to be included in production code. But I'm not sure that would be clear to other readers. Also, it's not clear when to do abort() and when to do CRASH(). We should create some guidelines for this.

> +#if ENABLE(JSC_MULTIPLE_THREADS)
> +        instance->makeUsableFromMultipleThreads();
> +#endif

In general, when the semantics of a function like this is simple, I prefer to make empty inline versions so we don't need to make each call site conditional. That would have prevented us from needing conditionals in JSGlobalData.h and here in JSGlobalData.cpp and in JSContextRef.cpp.

What do you think of that practice?

r=me as is
Comment 3 Alexey Proskuryakov 2008-11-20 10:13:53 PST
Committed revision 38622.

(In reply to comment #2)
> Also, it's not clear when to do abort() and when to do CRASH(). We
> should create some guidelines for this.

I don't like calling abort(), because it doesn't give any information for a bug report. Also, I think it's impolite to silently close an application. as if it was never running.

> What do you think of that practice?

In general, it's fine, but I didn't want code that's not guarded with ENABLE(JSC_MULTIPLE_THREADS) to call makeUsableFromMultipleThreads(), and I still don't want to.