Bug 16596 - ThreadSafeShared should be lock-free where possible
Summary: ThreadSafeShared should be lock-free where possible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-12-24 03:03 PST by Mark Rowe (bdash)
Modified: 2007-12-24 22:56 PST (History)
0 users

See Also:


Attachments
Fix Bug 16596: ThreadSafeShared should be lock-free where possible. (6.61 KB, patch)
2007-12-24 03:11 PST, Mark Rowe (bdash)
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2007-12-24 03:03:42 PST
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.
Comment 1 Mark Rowe (bdash) 2007-12-24 03:11:22 PST
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(-)
Comment 2 Alexey Proskuryakov 2007-12-24 08:06:01 PST
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.
Comment 3 Mark Rowe (bdash) 2007-12-24 13:47:57 PST
(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.
Comment 4 Sam Weinig 2007-12-24 14:14:51 PST
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.
Comment 5 Maciej Stachowiak 2007-12-24 15:45:24 PST
Also, how about an ARM version?
Comment 6 Mark Rowe (bdash) 2007-12-24 17:51:40 PST
Maciej, I have no way to test an ARM version so I will leave that for someone that can.
Comment 7 Mark Rowe (bdash) 2007-12-24 22:56:00 PST
Landed in r28979.