Bug 16596 – ThreadSafeShared should be lock-free where possible

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

[Mark Rowe (bdash)]

Using Mutex around refcounting operations is very heavy-weight and incredibly slow when there is contention.  It should use atomic increment and decrement-and-test on platforms that support it to improve performance.

[Mark Rowe (bdash)]

Created attachment 18091 [details]
Fix Bug 16596: ThreadSafeShared should be lock-free where possible.

 WebCore/ChangeLog            |   22 +++++++
 WebCore/platform/Threading.h |  133 +++++++++++++++++++++++++++++++++++------
 2 files changed, 135 insertions(+), 20 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 18091 [details]
Fix Bug 16596: ThreadSafeShared should be lock-free where possible.

+#if COMPILER(MSVC)
+#include <windows.h>
+#endif

I think (though I'm not quite sure) that we avoid including windows.h in headers to reduce conflicts.

Also, why is the check for COMPILER(MSVC), not PLATFORM(WIN_OS) or PLATFORM(WIN)?

+inline void InterlockedIncrement(int volatile* addend)
+{
+#if PLATFORM(X86) || PLATFORM(X86_64)
+    __asm__(
+        "lock\n\t"

FWIW, OS X also has atomic functions, e.g. AddAtomic and DecrementAtomic. I'm not sure if these are better or even suitable, but since you use an OS-provided function on Windows, there should at least be a comment explaining why we don't do the same on the Mac.

[Mark Rowe (bdash)]

(In reply to comment #2)
> (From update of attachment 18091 [details] [edit])
> +#if COMPILER(MSVC)
> +#include <windows.h>
> +#endif
> 
> I think (though I'm not quite sure) that we avoid including windows.h in
> headers to reduce conflicts.
> 
> Also, why is the check for COMPILER(MSVC), not PLATFORM(WIN_OS) or
> PLATFORM(WIN)?

I guess PLATFORM(WN) may be more appropriate.  I don't see how I can avoid including windows.h here given that the header file uses a function it declares.

> 
> +inline void InterlockedIncrement(int volatile* addend)
> +{
> +#if PLATFORM(X86) || PLATFORM(X86_64)
> +    __asm__(
> +        "lock\n\t"
> 
> FWIW, OS X also has atomic functions, e.g. AddAtomic and DecrementAtomic. I'm
> not sure if these are better or even suitable, but since you use an OS-provided
> function on Windows, there should at least be a comment explaining why we don't
> do the same on the Mac.

Two reasons I didn't use them: 1) They won't be inlined quite as nicely like these implementations are due to them consisting of a function call, 2) These work on non-Mac platforms.   The Win32 version will use a compiler intrinsic if it's available which will be inlined, and that avoids having to write a second version of the x86 code in MSVC-friendly form.

[Sam Weinig]

Comment on attachment 18091 [details]
Fix Bug 16596: ThreadSafeShared should be lock-free where possible.

All in all, this seems great.  I do have a few nits though.

Typo.  This should be 1.7x.
+        This is a 1.7xs as fast as the lock-based implementation on x86 for a single-threaded use

As discussed on IRC, #if PLATFORM(WIN_OS) might be better than #if COMPILER(MSVC) for this.  And I think it would be clearer if you didn't reuse the names InterlockedIncrement/InterlockedDecrement but rather used a new inline function atomicIncrement/atomicDecrement that forwarded to those calls on windows.

[Maciej Stachowiak]

Also, how about an ARM version?

[Mark Rowe (bdash)]

Maciej, I have no way to test an ARM version so I will leave that for someone that can.

[Mark Rowe (bdash)]

Landed in r28979.