Bug 88957 – ARMv7 should support spinlocks

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

ARMv7 should support spinlocks

[Geoffrey Garen]

Created attachment 147228 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/At..." exit_code: 1
Source/WTF/wtf/TCSpinLock.h:44:  TCMalloc_SlowLock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WTF/wtf/TCSpinLock.h:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WTF/wtf/TCSpinLock.h:54:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WTF/wtf/TCSpinLock.h:70:  lockword_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WTF/wtf/TCSpinLock.h:75:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WTF/wtf/TCSpinLock.h:75:  TCMalloc_SlowLock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WTF/wtf/TCSpinLock.h:76:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WTF/wtf/TCSpinLock.h:83:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 4 files


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

[Darin Adler]

Comment on attachment 147228 [details]
Patch

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

Looks OK to me.

Seems like an even better approach would be to make our own spin lock in WTF and then have TCMalloc_SpinLock be a thin cover on that to try to avoid this mix of TCMalloc code in a different style with our own original WTF code. I assume it would be better for reuse as well.

But this patch seems fine to land as-is.

> Source/WTF/wtf/Platform.h:1073
> -#if !defined(ENABLE_COMPARE_AND_SWAP) && COMPILER(GCC) && (CPU(X86) || CPU(X86_64) || CPU(ARM_THUMB2))
> +#if !defined(ENABLE_COMPARE_AND_SWAP) && (CPU(X86) || CPU(X86_64) || CPU(ARM_THUMB2))
>  #define ENABLE_COMPARE_AND_SWAP 1
>  #endif

The comment about this change says “enable on Windows”, but the code change says “enable on all non-GCC compilers”. Which do you want to do?

>> Source/WTF/wtf/TCSpinLock.h:48
>> +    inline void Lock() {
> 
> Place brace on its own line for function definitions.  [whitespace/braces] [4]

Since you changed the indenting, you could also remove the needless “inline” that does nothing here. Hard to know how much to leave this like the original code.

> Source/WTF/wtf/TCSpinLock.h:51
> +      if (!WTF::weakCompareAndSwap(&lockword_, 0, 1))
> +        TCMalloc_SlowLock(&lockword_);
> +      WTF::memoryBarrierAfterLock();

Given the WTF approach to namespaces, normally we need the WTF prefix only if we forgot some using statements in the header. Ideally we would pick a consistent style instead of mixing styles.

>> Source/WTF/wtf/TCSpinLock.h:54
>> +    inline void Unlock() {
> 
> Place brace on its own line for function definitions.  [whitespace/braces] [4]

Same comment about inline.

> Source/WTF/wtf/TCSpinLock.h:55
> +      WTF::memoryBarrierBeforeUnlock();

Same comment about WTF.

[Geoffrey Garen]

> Given the WTF approach to namespaces, normally we need the WTF prefix only if we forgot some using statements in the header. Ideally we would pick a consistent style instead of mixing styles.

Should the WTF file "export" its functions with using declarations, or should the spin lock file "import" WTF functions with a "using namespace WTF"?

[Geoffrey Garen]

> Seems like an even better approach would be to make our own spin lock in WTF

Yes, this is better. I will refactor to this eventually. Thinking about memory ordering and adding files to our many different build systems at the same time was too much for my feeble mind.

> > Source/WTF/wtf/Platform.h:1073
> > -#if !defined(ENABLE_COMPARE_AND_SWAP) && COMPILER(GCC) && (CPU(X86) || CPU(X86_64) || CPU(ARM_THUMB2))
> > +#if !defined(ENABLE_COMPARE_AND_SWAP) && (CPU(X86) || CPU(X86_64) || CPU(ARM_THUMB2))
> >  #define ENABLE_COMPARE_AND_SWAP 1
> >  #endif
> 
> The comment about this change says “enable on Windows”, but the code change says “enable on all non-GCC compilers”. Which do you want to do?

Updated to list windows and then GCC+platforms.

> >> Source/WTF/wtf/TCSpinLock.h:48
> >> +    inline void Lock() {
> > 
> > Place brace on its own line for function definitions.  [whitespace/braces] [4]
> 
> Since you changed the indenting, you could also remove the needless “inline” that does nothing here. Hard to know how much to leave this like the original code.

Fixed.

[Geoffrey Garen]

Committed r120356: <http://trac.webkit.org/changeset/120356>