Bug 169300 – WTF should make it super easy to do ARM concurrency tricks

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Filip Pizlo]

Patch forthcoming.

So far I've been able to make it so that this code:

                    lock.transaction(
                        [&] (LockType& value) -> bool {
                            value &= ~mask;
                            if (result.mayHaveMoreThreads)
                                value |= hasParkedBit;
                            return true;
                        });

Emits this code:

0000000000000338	ldaxrb	w9, [x8]
000000000000033c	and	w9, w9, #0xfffffffc
0000000000000340	orr	w9, w9, #0x2
0000000000000344	stlxrb	w10, w9, [x8]
0000000000000348	tbnz	w10, #0x0, 0x338

That's some nice LL/SC code!

[Filip Pizlo]

Created attachment 303709 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/Atomics.h:116:  The parameter name "order" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Atomics.h:117:  The parameter name "order" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Atomics.h:186:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/Atomics.h:188:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:189:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:189:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Platform.h:762:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 7 in 7 files


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

[Mark Lam]

Comment on attachment 303709 [details]
the patch

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

r=me

> Source/WTF/wtf/Atomics.h:125
> +        return load(std::memory_order_relaxed);

Need UNUSED_PARAM(order) here.

> Source/WTF/wtf/Atomics.h:184
> +        int ## width ## _t result; \

/int/uint/ to match return type.

[Filip Pizlo]

Oh man, this is a perf regression.  But it inspired me to look at GC codegen.

[Filip Pizlo]

Created attachment 303763 [details]
more

This is a crazy patch.  I sort of went on an optimization walkabout.

[Filip Pizlo]

Created attachment 303769 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/Atomics.h:121:  The parameter name "order" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Atomics.h:122:  The parameter name "order" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Atomics.h:188:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/Atomics.h:190:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:191:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:191:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:445:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:451:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:445:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:451:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Platform.h:762:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 11 in 16 files


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

[Keith Miller]

Bots don't look happy.

[Filip Pizlo]

Created attachment 303773 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/Atomics.h:121:  The parameter name "order" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Atomics.h:122:  The parameter name "order" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Atomics.h:188:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/Atomics.h:190:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:191:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:191:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:445:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:451:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:445:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:451:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Platform.h:762:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 11 in 16 files


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

[Mark Lam]

Comment on attachment 303773 [details]
the patch

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

> Source/WTF/ChangeLog:53
> +        (WTF::consumeLoad): Deleted.

Since you deleted this, you also need to delete the corresponding test from the API tests.  It looks like this is what's causing the GTK bot to be sad.  You should also check to see if the other deleted functions have corresponding tests.

[Filip Pizlo]

(In reply to comment #11)
> Comment on attachment 303773 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303773&action=review
> 
> > Source/WTF/ChangeLog:53
> > +        (WTF::consumeLoad): Deleted.
> 
> Since you deleted this, you also need to delete the corresponding test from
> the API tests.  It looks like this is what's causing the GTK bot to be sad. 
> You should also check to see if the other deleted functions have
> corresponding tests.

Oh man, I almost got a perfect score on EWS!  That's what I get for not building all of WK.  Will do that now, and use that as an opportunity to measure JetStream perf.

[Filip Pizlo]

Created attachment 303845 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/Atomics.h:121:  The parameter name "order" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Atomics.h:122:  The parameter name "order" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Atomics.h:188:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/Atomics.h:190:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:191:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:191:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:445:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:451:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:445:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:451:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Platform.h:762:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 11 in 17 files


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

[Mark Lam]

Comment on attachment 303845 [details]
the patch

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

> Source/JavaScriptCore/heap/SlotVisitor.cpp:-247
> -    SuperSamplerScope superSamplerScope(false);
> -    

Should this be deleted, or should it be moved up to SlotVisitor::appendSlow() and SlotVisitor::appendHiddenSlow()?

> Source/JavaScriptCore/heap/SlotVisitorInlines.h:64
> +    appendHiddenSlow(cell, dependency);

I think this should be appendSlow() because we need to be able to handle the m_heapSnapshotBuilder case.

> Source/WTF/wtf/Atomics.h:125
> +    // Props to 

What is this comment trying to say?

> Source/WTF/wtf/Atomics.h:151
> +    

This added space indentation is unneeded.

> Source/WTF/wtf/Atomics.h:456
>  template <typename T, typename std::enable_if<sizeof(T) == 4>::type* = nullptr>
> -ALWAYS_INLINE ConsumeDependency zeroWithConsumeDependency(T value)
> +ALWAYS_INLINE Dependency dependency(T value)
>  {
> -    uint32_t dependency;
> +    unsigned dependency;
>      uint32_t copy = bitwise_cast<uint32_t>(value);
>  #if CPU(ARM64)
>      asm volatile("eor %w[dependency], %w[in], %w[in]"

The only difference between this <sizeof(T) == 4> version and the <sizeof(T) == 8> above is that the inline asm in this one is declared "volatile" and touching "memory".  If these are not needed for the <sizeof(T) == 8> above, then they are not needed here.  As a result, do we really need separate <sizeof(T) == 4> and <sizeof(T) == 8> versions?  Both returns an unsigned sized Dependency anyway.

[Filip Pizlo]

(In reply to comment #15)
> Comment on attachment 303845 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303845&action=review
> 
> > Source/JavaScriptCore/heap/SlotVisitor.cpp:-247
> > -    SuperSamplerScope superSamplerScope(false);
> > -    
> 
> Should this be deleted, or should it be moved up to
> SlotVisitor::appendSlow() and SlotVisitor::appendHiddenSlow()?

It doesn't do anything unless you change `false` to `true`.  So, I think we should just remove these things whenever they are in the way of a refactoring.

> 
> > Source/JavaScriptCore/heap/SlotVisitorInlines.h:64
> > +    appendHiddenSlow(cell, dependency);
> 
> I think this should be appendSlow() because we need to be able to handle the
> m_heapSnapshotBuilder case.

Right!  Fixed!

> 
> > Source/WTF/wtf/Atomics.h:125
> > +    // Props to 
> 
> What is this comment trying to say?

Nah.  I was going to cite where I learned the prepare/attempt jargon from, but I think I found that the origin is ambiguous.

> 
> > Source/WTF/wtf/Atomics.h:151
> > +    
> 
> This added space indentation is unneeded.

Removed.

> 
> > Source/WTF/wtf/Atomics.h:456
> >  template <typename T, typename std::enable_if<sizeof(T) == 4>::type* = nullptr>
> > -ALWAYS_INLINE ConsumeDependency zeroWithConsumeDependency(T value)
> > +ALWAYS_INLINE Dependency dependency(T value)
> >  {
> > -    uint32_t dependency;
> > +    unsigned dependency;
> >      uint32_t copy = bitwise_cast<uint32_t>(value);
> >  #if CPU(ARM64)
> >      asm volatile("eor %w[dependency], %w[in], %w[in]"
> 
> The only difference between this <sizeof(T) == 4> version and the <sizeof(T)
> == 8> above is that the inline asm in this one is declared "volatile" and
> touching "memory".  If these are not needed for the <sizeof(T) == 8> above,
> then they are not needed here.  As a result, do we really need separate
> <sizeof(T) == 4> and <sizeof(T) == 8> versions?  Both returns an unsigned
> sized Dependency anyway.

I removed volatile/"memory" - it's not needed for either.

There's still one more difference - bitwise casting to uint64_t versus uint32_t.  I can see a few ways to refactor this to combine code, but it's a bit tricky, so I'll file a bug and add a FIXME.

[Mark Lam]

Comment on attachment 303845 [details]
the patch

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

r=me with fixes applied.

>>> Source/WTF/wtf/Atomics.h:456
>>>      asm volatile("eor %w[dependency], %w[in], %w[in]"
>> 
>> The only difference between this <sizeof(T) == 4> version and the <sizeof(T) == 8> above is that the inline asm in this one is declared "volatile" and touching "memory".  If these are not needed for the <sizeof(T) == 8> above, then they are not needed here.  As a result, do we really need separate <sizeof(T) == 4> and <sizeof(T) == 8> versions?  Both returns an unsigned sized Dependency anyway.
> 
> I removed volatile/"memory" - it's not needed for either.
> 
> There's still one more difference - bitwise casting to uint64_t versus uint32_t.  I can see a few ways to refactor this to combine code, but it's a bit tricky, so I'll file a bug and add a FIXME.

Ah yes, I missed that part.  I agree the refactoring need not happen in this patch.

[Filip Pizlo]

Created attachment 303899 [details]
the patch

[Filip Pizlo]

Comment on attachment 303899 [details]
the patch

r=mlam

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/Atomics.h:121:  The parameter name "order" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Atomics.h:122:  The parameter name "order" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Atomics.h:187:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/Atomics.h:189:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:190:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:190:  Wrong number of spaces before statement. (expected: 20)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:444:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:449:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:452:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:444:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:449:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:452:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:467:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:473:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:467:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:473:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Platform.h:762:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 17 in 17 files


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

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/213645

[Csaba Osztrogonác]

(In reply to comment #21)
> Landed in https://trac.webkit.org/changeset/213645

It broke the JSCOnly AArch64 build:

../../Source/WTF/wtf/Atomics.h:238:64: error: redefinition of 'T WTF::Atomic<T>::loadLink(std::memory_order) [with T = long unsigned int; std::memory_order = std::memory_order]'
In file included from ../../Source/WTF/wtf/Threading.h:40:0,
                 from ../../Source/WTF/wtf/CompilationThread.cpp:31:
../../Source/WTF/wtf/Atomics.h:237:63: note: 'T WTF::Atomic<T>::loadLink(std::memory_order) [with T = long unsigned int; std::memory_order = std::memory_order]' previously declared here
In file included from ../../Source/WTF/wtf/Threading.h:40:0,
                 from ../../Source/WTF/wtf/CompilationThread.cpp:31:

[Filip Pizlo]

(In reply to comment #22)
> (In reply to comment #21)
> > Landed in https://trac.webkit.org/changeset/213645
> 
> It broke the JSCOnly AArch64 build:
> 
> ../../Source/WTF/wtf/Atomics.h:238:64: error: redefinition of 'T
> WTF::Atomic<T>::loadLink(std::memory_order) [with T = long unsigned int;
> std::memory_order = std::memory_order]'
> In file included from ../../Source/WTF/wtf/Threading.h:40:0,
>                  from ../../Source/WTF/wtf/CompilationThread.cpp:31:
> ../../Source/WTF/wtf/Atomics.h:237:63: note: 'T
> WTF::Atomic<T>::loadLink(std::memory_order) [with T = long unsigned int;
> std::memory_order = std::memory_order]' previously declared here
> In file included from ../../Source/WTF/wtf/Threading.h:40:0,
>                  from ../../Source/WTF/wtf/CompilationThread.cpp:31:

Right - because on Linux, intptr_t and int64_t are typedefs for the same thing.

Do you know if there is a #define that we could case on, that tells us if intptr_t and int64_t are the same or different types?  If there isn't such a thing, then maybe guarding this with OS(DARWIN) would be enough: 

#if OS(DARWIN)
DEFINE_LL_SC(ptr, "", "")
#endif

[Csaba Osztrogonác]

(In reply to comment #23)

> Right - because on Linux, intptr_t and int64_t are typedefs for the same
> thing.
> 
> Do you know if there is a #define that we could case on, that tells us if
> intptr_t and int64_t are the same or different types?  If there isn't such a
> thing, then maybe guarding this with OS(DARWIN) would be enough: 
> 
> #if OS(DARWIN)
> DEFINE_LL_SC(ptr, "", "")
> #endif

Unfortunately there is no way to check if intptr_t and int64_t are
same types or not in preprocess time. But they are always same on
Linux (64-bit), so the easiest way is to add a DARWIN guard here.

Fix landed in https://trac.webkit.org/changeset/213705

[Csaba Osztrogonác]

It completely broke JSC on AArch64 Linux, please see bug169510 for details.

[Yusuke Suzuki]

(In reply to comment #25)
> It completely broke JSC on AArch64 Linux, please see bug169510 for details.

In the meantime, can we fix it by chainging

#if CPU(ARM64)
#define HAVE_LL_SC 1
#endif // CPU(ARM64)

in Platform.h to

#if OS(DARWIN) && CPU(ARM64)
#define HAVE_LL_SC 1
#endif // OS(DARWIN) && CPU(ARM64)