Bug 16464 – Modify WebCore to use win32 thread primitives

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

This Bug complements Bug #16428.  It removes the need on Windows builds to link against (and ship) the pthreads threading library.

The patches are broken into two parts.
1.  A patch to add a windows-specific thread implementation to WebCore/platforms/win.
2.  A patch to modify the config, Threading header, and build project to actually build and link against the win32 threading primitives.

The first patch should be safe to apply as it just adds a source file.  The second patch should be tested carefully before accepting.

[Brent Fulgham]

Created attachment 17944 [details]
Patch to add win32-based threading implementation.

This first patch just adds a new source file to the project.  It does not modify the build or change any build commands or configuration.

[Brent Fulgham]

Created attachment 17945 [details]
Patch to add win32-based threading implementation.

This patch just adds a single file to the platform/win directory that implements a win32-based threading implementation.

[Brent Fulgham]

Created attachment 17946 [details]
Patch to add win32-based threading implementation (modifications to build system)

This second patch modifies the build environment to switch to the native win32 threading primitives.  It seems to work well when testing with database-based pages (such as http://webkit.org/misc/DatabaseExample.html).  This patch should be carefully reviewed!

[Maciej Stachowiak]

I'm a little concerned at how many thread operations grab a global mutex. But I guess this is carried over from the original.

[Brent Fulgham]

Created attachment 17950 [details]
Patch to add win32-based threading implementation (includes changes to build system).

Revised patches based on first round of comments from reviewers.

[Darin Adler]

Comment on attachment 17950 [details]
Patch to add win32-based threading implementation (includes changes to build system).

Looks good.

I would have expected the Mutex.cpp file to be named MutexWin.cpp. We've been using both filenames and directory names to distinguish these platform-specific files.

These types with _t in their names aren't needed. It can just be:

    struct PlatformMutex { };

Rather than:

    struct PlatformMutex_t { };
    typedef PlatformMutex_t PlatformMutex.

And in fact the extra types don't do any good.

Also, we don't normally format members with their names lined up at tab stops.

 53     // Warning:  pthread_mutex_trylock will return an error
 54     // if the lock is already owned by the current thread.  Win32
 55     // threads make no such complaint.

This comment will be confusing in the future. At the moment you are writing this, pthreads seems relevant, but later people will wonder why you mention it. I think you need to say more clearly that this is modeled after the pthread_mutex_trylock semantic, and why it is.

 68         return true;
 69     } else if (result == 0)   // ... failed

We normally don't do else after return. Further, there's no need for the else to check the opposite of the if condition. The style we normally use is "early exit". If you straighten out the logic here there won't be an unreached code that uses ASSERT(false). By the way, when it does arise we write that as ASSERT_NOT_REACHED().

If you're going to quote the Boost license, it needs to be at the top of the file with the other license, not inside the contents.

 70 static const long MAX_SEMAPHORE_COUNT = ((int) ((~0u) >> 1));

We normally do not use the ALL_CAPS form for things that are not macros. Also, since this constant is a long, it doesn't really make sense that it uses unsigned and int in its declaration. It should also use static_cast rather than C-style cast and fewer extraneous parentheses and it would be easier to read.

 102     // Enter the wait state...

I'm not a big fan of "..." in cases like this. A normal period would do as well.

3232 #include "Logging.h"
3333 #include "Page.h"
 34 
 35 #include <wtf/HashMap.h>
3436 #include <windows.h>
 37 #include <errno.h>

We sort includes alphabetically and we don't put spaces between the quoted and angle bracket ones.

 66 static ThreadIdentifier identifierByThreadHandle(const HANDLE threadHandle)

The const here doesn't mean anything useful. It just says that this argument isn't modified inside the function and there's no reason for this particular function to have it.

 72         const HANDLE currHandle = i->second;

The const here also has limited value.

 74             return (ThreadIdentifier)i->first;

This should use static_cast. But I'm surprised a cast is needed at all. This may be a problem under 64-bit.

 88     MutexLocker locker(threadMapMutex());
 89 
 90     ASSERT(threadMap().contains(id));
 91     
 92     threadMap().remove(id);

Would read better without the blank lines.

 101     if (0 == threadHandle) {

We use "!threadHandle" for cases like this.

 106     threadID = (ThreadIdentifier)threadIdentifier;

And static_cast here.

 143     return (ThreadIdentifier)::GetCurrentThreadId ();

And here, and no space before the () in the function call.

These points of style are relatively minor, but review- so we can get them fixed. The most important one is probably the extra code paths in Mutex::tryLock.

[Brent Fulgham]

Created attachment 17965 [details]
Updated based on review.

Combined patch to move WebCore from pthreads emulation library to native Win32 threading primitives.

[Brent Fulgham]

Comment on attachment 17965 [details]
Updated based on review.

Updated based on darin's review.

[Darin Adler]

Comment on attachment 17965 [details]
Updated based on review.

 71 static const long MaxSemaphoreCount = static_cast<long>((~0u) >> 1);

I think it needs to be ~0UL rather than ~0U. And also there is no need for parentheses there -- the precedence of the ~ operator is tighter than the >> operator, and I think it's easier to read as is.

Patch has some tabs in it. We can't check in patches with tabs.

 115     unsigned was_gone=0;

Unused local variable here.

Otherwise looks good. r=me, although I'm a little nervous about the level of testing that will be required to vet this on Windows with Safari.

[Brent Fulgham]

Created attachment 17967 [details]
Patch to add win32-based threading implementation (includes changes to build system).

Revised again to remove unused local variable, change "~0u" to "~0ul", and to remove tab characters.

[Darin Adler]

Comment on attachment 17967 [details]
Patch to add win32-based threading implementation (includes changes to build system).

r=me

[Alp Toker]

Landed in r28835.