Bug 114563 – [WIN] Remove remaining calls to pthread from WTF

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Patrick R. Gansterer]

[WIN] Remove last calls to pthread from WTF

[Patrick R. Gansterer]

Created attachment 197937 [details]
Patch

[WebKit Commit Bot]

Attachment 197937 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/FastMalloc.cpp', u'Source/WTF/wtf/ThreadSpecificWin.cpp']" exit_code: 1
Source/WTF/wtf/FastMalloc.cpp:2845:  heap_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/FastMalloc.cpp:3434:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 197937 [details]
Patch

Attachment 197937 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/143154

[Build Bot]

Comment on attachment 197937 [details]
Patch

Attachment 197937 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/148130

[Patrick R. Gansterer]

Created attachment 197943 [details]
Patch

[WebKit Commit Bot]

Attachment 197943 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/FastMalloc.cpp', u'Source/WTF/wtf/ThreadSpecificWin.cpp']" exit_code: 1
Source/WTF/wtf/FastMalloc.cpp:2845:  heap_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/FastMalloc.cpp:3440:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 197943 [details]
Patch

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

> Source/WTF/wtf/ThreadSpecificWin.cpp:43
> -    DEFINE_STATIC_LOCAL(DoublyLinkedList<PlatformThreadSpecificKey>, staticList, ());
> +    static DoublyLinkedList<PlatformThreadSpecificKey> staticList;
>      return staticList;
>  }
>  
>  static Mutex& destructorsMutex()
>  {
> -    DEFINE_STATIC_LOCAL(Mutex, staticMutex, ());
> +    static Mutex staticMutex;

Why this change?

[Brent Fulgham]

Comment on attachment 197943 [details]
Patch

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

This change looks good to me, and the EWS seems happy.  If this change is committed, do we get to remove the pthreads library from Windows?

> Source/WTF/wtf/FastMalloc.cpp:-2864
> -    TlsSetValue(tlsIndex, heap);

Don't we still need to use TlsSetValue?  That's part of WinAPI IIRC. Or does threadSpecificSet deal with all the different platforms properly?

>> Source/WTF/wtf/ThreadSpecificWin.cpp:43
>> +    static Mutex staticMutex;
> 
> Why this change?

I think it's because DEFINE_STATIC_LOCAL(type, name, arguments) is defined as "static type& name = *new type arguments".

> Source/WTF/wtf/ThreadSpecificWin.cpp:96
> +    new (*key) PlatformThreadSpecificKey(destructor);

Is placement new supported on all of our compilers?

[WebKit Commit Bot]

Comment on attachment 197943 [details]
Patch

Clearing flags on attachment: 197943

Committed r148462: <http://trac.webkit.org/changeset/148462>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Patrick R. Gansterer]

(In reply to comment #8)
> (From update of attachment 197943 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197943&action=review
> 
> This change looks good to me, and the EWS seems happy.  If this change is committed, do we get to remove the pthreads library from Windows?

For JavaScriptCore.dll and WebKit.dll it should be possible now, but I didn't try it with the "offical build flags" (ENABLE_*). DRT needs at least bug 92505 and bug 92371 fixed.

> > Source/WTF/wtf/FastMalloc.cpp:-2864
> > -    TlsSetValue(tlsIndex, heap);
> 
> Don't we still need to use TlsSetValue?  That's part of WinAPI IIRC. Or does threadSpecificSet deal with all the different platforms properly?

It should be possible to use threadSpecificSet() for all platforms, since it provides exactly this abstraction.

> >> Source/WTF/wtf/ThreadSpecificWin.cpp:43
> >> +    static Mutex staticMutex;
> > 
> > Why this change?
> 
> I think it's because DEFINE_STATIC_LOCAL(type, name, arguments) is defined as "static type& name = *new type arguments".

Yup.

 
> > Source/WTF/wtf/ThreadSpecificWin.cpp:96
> > +    new (*key) PlatformThreadSpecificKey(destructor);
> 
> Is placement new supported on all of our compilers?

Since we use at many other places already, I would say so. :-)

[Ryosuke Niwa]

This patch broke Windows builds :(

http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/47526

[WebKit Commit Bot]

Re-opened since this is blocked by bug 114658

[Brent Fulgham]

(In reply to comment #13)
> Re-opened since this is blocked by bug 114658

I'm skeptical that this change created the build failure.  It doesn't touch JSC or export behavior.

Strange!

I'm trying a Windows build locally to see what happens.

[Ryosuke Niwa]

Odd. The build failed even after the rollout so you're probably right that this patch isn't the culprit. I've triggered a clean build to see what happens.

[Ryosuke Niwa]

Re-landed in http://trac.webkit.org/changeset/148490.

[Brent Fulgham]

I tried removing the 'pthreads' include path as a step to excise it from the Windows build, but started getting include errors because the pthread header is used when FORCE_SYSTEM_MALLOC is 0.

This will be zero if USE_SYSTEM_MALLOC is unset.

This change probably works great for WinCE, since USE_SYSTEM_MALLOC is always set, but I'm not sure what the preferred approach is for Apple's Windows port.

[Patrick R. Gansterer]

(In reply to comment #17)
> I tried removing the 'pthreads' include path as a step to excise it from the Windows build, but started getting include errors because the pthread header is used when FORCE_SYSTEM_MALLOC is 0.
> 
> This will be zero if USE_SYSTEM_MALLOC is unset.
> 
> This change probably works great for WinCE, since USE_SYSTEM_MALLOC is always set, but I'm not sure what the preferred approach is for Apple's Windows port.

Why not add a USE(PTHREADS) around the bad include?

[Roger Fong]

<rdar://problem/13852055>