Summary: | ThreadSafeShared should be lock-free where possible | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Rowe (bdash) <mrowe> | ||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Mac | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Mark Rowe (bdash)
2007-12-24 03:03:42 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 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. (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 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. Also, how about an ARM version? Maciej, I have no way to test an ARM version so I will leave that for someone that can. |