[webkit-reviews] review granted: [Bug 22364] Crashes seen on Tiger buildbots due to worker threads exhausting pthread keys : [Attachment 25307] proposed fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 22364: Crashes seen on Tiger buildbots due to worker threads exhausting
pthread keys
https://bugs.webkit.org/show_bug.cgi?id=22364

Attachment 25307: proposed fix
https://bugs.webkit.org/attachment.cgi?id=25307&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>  #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