[webkit-reviews] review granted: [Bug 88957] ARMv7 should support spinlocks : [Attachment 147228] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 88957: ARMv7 should support spinlocks
https://bugs.webkit.org/show_bug.cgi?id=88957

Attachment 147228: Patch
https://bugs.webkit.org/attachment.cgi?id=147228&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.