Bug 130320 - [Mac] WTFThreadData should use _pthread_getspecific_direct().
Summary: [Mac] WTFThreadData should use _pthread_getspecific_direct().
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-17 00:03 PDT by Andreas Kling
Modified: 2014-03-17 10:08 PDT (History)
8 users (show)

See Also:


Attachments
Patch idea (3.70 KB, patch)
2014-03-17 00:11 PDT, Andreas Kling
darin: review+
Details | Formatted Diff | Diff
Patch for landing (4.33 KB, patch)
2014-03-17 00:50 PDT, Andreas Kling
kling: commit-queue+
Details | Formatted Diff | Diff
Patch for landing (4.27 KB, patch)
2014-03-17 00:52 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2014-03-17 00:03:29 PDT
[Mac] WTFThreadData should use _pthread_getspecific_direct().
Comment 1 Andreas Kling 2014-03-17 00:11:22 PDT
Created attachment 226890 [details]
Patch idea
Comment 2 Darin Adler 2014-03-17 00:39:58 PDT
Comment on attachment 226890 [details]
Patch idea

View in context: https://bugs.webkit.org/attachment.cgi?id=226890&action=review

Looks good.

> Source/WTF/wtf/WTFThreadData.cpp:77
> +#if USE(PTHREAD_GETSPECIFIC_DIRECT)
> +static void destroyWTFThreadData(void* data)

Should leave another blank line here to match the one we left below before #endif.

Also could consider using a lambda instead of declaring a separate function. Not sure what is preferred style.

> Source/WTF/wtf/WTFThreadData.h:47
> +#ifdef __has_include
> +#if __has_include(<System/pthread_machdep.h>)
> +
> +#include <System/pthread_machdep.h>
> +
> +#if defined(__PTK_FRAMEWORK_JAVASCRIPTCORE_KEY0)
> +#define WTF_USE_PTHREAD_GETSPECIFIC_DIRECT 1
> +#endif
> +
> +#endif
> +#endif

I don’t like nested #if, so I’d write it like this:

#if defined(__has_include) && __has_include(<System/pthread_machdep.h>)
#include <System/pthread_machdep.h>
#endif

#if defined(__PTK_FRAMEWORK_JAVASCRIPTCORE_KEY1)
#define WTF_USE_PTHREAD_GETSPECIFIC_DIRECT 1
#endif

I think we should make the same improvement in FastMalloc.cpp; just easier to read without the nesting. But also, I am not sure we should use the same USE(PTHREAD_GETSPECIFIC_DIRECT) both here and there; is there a risk of conflict if we compile all-in-one or something?

The reason I suggest checking for __PTK_FRAMEWORK_JAVASCRIPTCORE_KEY1 instead of __PTK_FRAMEWORK_JAVASCRIPTCORE_KEY0 is that is the key we are actually using here. Which I suppose doesn’t make sense if this is a single USE for both places.

> Source/WTF/wtf/WTFThreadData.h:159
> +#if USE(PTHREAD_GETSPECIFIC_DIRECT)

Might consider putting the portable side of the #if first rather than the platform-specific one.

> Source/WTF/wtf/WTFThreadData.h:177
>      // WRT WebCore:

I think this comment applies to both sides of the #if, so it should be outside the #if.
Comment 3 Andreas Kling 2014-03-17 00:50:52 PDT
Created attachment 226893 [details]
Patch for landing

Tweaked per Darin's recipe.
Comment 4 Andreas Kling 2014-03-17 00:52:00 PDT
Created attachment 226894 [details]
Patch for landing
Comment 5 Darin Adler 2014-03-17 00:54:00 PDT
Comment on attachment 226894 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=226894&action=review

> Source/WTF/wtf/WTFThreadData.h:160
> +#if USE(PTHREAD_GETSPECIFIC_DIRECT)
> +    static const pthread_key_t directKey = __PTK_FRAMEWORK_JAVASCRIPTCORE_KEY1;
> +    WTF_EXPORT_PRIVATE static WTFThreadData& createAndRegisterForGetspecificDirect();
> +#else
>      static WTF_EXPORTDATA ThreadSpecific<WTFThreadData>* staticData;
> +#endif

Portable first could apply here too.
Comment 6 WebKit Commit Bot 2014-03-17 01:29:40 PDT
Comment on attachment 226894 [details]
Patch for landing

Clearing flags on attachment: 226894

Committed r165725: <http://trac.webkit.org/changeset/165725>
Comment 7 WebKit Commit Bot 2014-03-17 01:29:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Csaba Osztrogonác 2014-03-17 03:06:59 PDT
(In reply to comment #6)
> (From update of attachment 226894 [details])
> Clearing flags on attachment: 226894
> 
> Committed r165725: <http://trac.webkit.org/changeset/165725>

And buildfixes landed in 
 - http://trac.webkit.org/changeset/165726
 - http://trac.webkit.org/changeset/165729

It would have been better to let the EWS run before landing the patch.
Comment 9 Csaba Osztrogonác 2014-03-17 03:16:30 PDT
(In reply to comment #2)
> > Source/WTF/wtf/WTFThreadData.h:47
> > +#ifdef __has_include
> > +#if __has_include(<System/pthread_machdep.h>)
> > +
> > +#include <System/pthread_machdep.h>
> > +
> > +#if defined(__PTK_FRAMEWORK_JAVASCRIPTCORE_KEY0)
> > +#define WTF_USE_PTHREAD_GETSPECIFIC_DIRECT 1
> > +#endif
> > +
> > +#endif
> > +#endif
> 
> I don’t like nested #if, so I’d write it like this:
> 
> #if defined(__has_include) && __has_include(<System/pthread_machdep.h>)
> #include <System/pthread_machdep.h>
> #endif

The idea was reasonable not to have nested ifs. But unfortunately it made
GCC unhappy:
/mnt/buildbot/efl-linux-slave-3/efl-linux-32-release/build/Source/WTF/wtf/WTFThreadData.h:37:44: error: missing binary operator before token "("

It seems short circuit evaluation doesn't work in GCC preprocessor
and its parser gets confused by the undefined __has_include() macro.

In this case only the nested #if works:
#ifdef __has_include
#if __has_include(...)
#include ...
#endif
#endif
Comment 10 Alexey Proskuryakov 2014-03-17 09:53:14 PDT
Tests started to crash with assertion failures around the time this was landed:

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r165732%20(3394)/svg/dom/transform-parser-crash-log.txt

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r165733%20(3842)/svg/W3C-SVG-1.1/shapes-polyline-01-t-crash-log.txt

Can this change be the culprit? Should we temporarily roll out to see if that fixes the crashes?
Comment 11 Alexey Proskuryakov 2014-03-17 10:08:42 PDT
Andreas found an instance of this crash happening prior to this change, and suspects bug 130317.