ARMv7 should support spinlocks
Created attachment 147228 [details] Patch
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.
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.
> 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"?
> 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.
Committed r120356: <http://trac.webkit.org/changeset/120356>