Bug 169300 - WTF should make it super easy to do ARM concurrency tricks
Summary: WTF should make it super easy to do ARM concurrency tricks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 169510
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-07 12:34 PST by Filip Pizlo
Modified: 2017-03-11 01:24 PST (History)
11 users (show)

See Also:


Attachments
the patch (11.21 KB, patch)
2017-03-07 13:07 PST, Filip Pizlo
mark.lam: review+
Details | Formatted Diff | Diff
more (34.86 KB, patch)
2017-03-07 19:47 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (39.42 KB, patch)
2017-03-07 20:31 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (39.77 KB, patch)
2017-03-07 21:10 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (49.46 KB, patch)
2017-03-08 14:29 PST, Filip Pizlo
mark.lam: review+
Details | Formatted Diff | Diff
the patch (50.01 KB, patch)
2017-03-08 22:33 PST, Filip Pizlo
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-03-07 12:34:24 PST
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!
Comment 1 Filip Pizlo 2017-03-07 13:07:14 PST
Created attachment 303709 [details]
the patch
Comment 2 WebKit Commit Bot 2017-03-07 13:08:20 PST
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.
Comment 3 Mark Lam 2017-03-07 13:24:32 PST
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.
Comment 4 Filip Pizlo 2017-03-07 19:46:19 PST
Oh man, this is a perf regression.  But it inspired me to look at GC codegen.
Comment 5 Filip Pizlo 2017-03-07 19:47:57 PST
Created attachment 303763 [details]
more

This is a crazy patch.  I sort of went on an optimization walkabout.
Comment 6 Filip Pizlo 2017-03-07 20:31:23 PST
Created attachment 303769 [details]
the patch
Comment 7 WebKit Commit Bot 2017-03-07 20:33:15 PST
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.
Comment 8 Keith Miller 2017-03-07 21:02:06 PST
Bots don't look happy.
Comment 9 Filip Pizlo 2017-03-07 21:10:10 PST
Created attachment 303773 [details]
the patch
Comment 10 WebKit Commit Bot 2017-03-07 21:12:58 PST
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.
Comment 11 Mark Lam 2017-03-07 21:29:43 PST
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.
Comment 12 Filip Pizlo 2017-03-08 11:31:59 PST
(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.
Comment 13 Filip Pizlo 2017-03-08 14:29:49 PST
Created attachment 303845 [details]
the patch
Comment 14 WebKit Commit Bot 2017-03-08 14:32:03 PST
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.
Comment 15 Mark Lam 2017-03-08 18:27:43 PST
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.
Comment 16 Filip Pizlo 2017-03-08 22:23:54 PST
(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.
Comment 17 Mark Lam 2017-03-08 22:27:49 PST
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.
Comment 18 Filip Pizlo 2017-03-08 22:33:43 PST
Created attachment 303899 [details]
the patch
Comment 19 Filip Pizlo 2017-03-08 22:35:05 PST
Comment on attachment 303899 [details]
the patch

r=mlam
Comment 20 WebKit Commit Bot 2017-03-08 22:36:23 PST
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.
Comment 21 Filip Pizlo 2017-03-09 09:40:38 PST
Landed in https://trac.webkit.org/changeset/213645
Comment 22 Csaba Osztrogonác 2017-03-09 13:50:12 PST
(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:
Comment 23 Filip Pizlo 2017-03-09 13:58:55 PST
(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
Comment 24 Csaba Osztrogonác 2017-03-10 02:33:59 PST
(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
Comment 25 Csaba Osztrogonác 2017-03-11 00:48:02 PST
It completely broke JSC on AArch64 Linux, please see bug169510 for details.
Comment 26 Yusuke Suzuki 2017-03-11 01:24:01 PST
(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)