Bug 169300

Created attachment 303709 [details] the patch

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 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.

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

Created attachment 303763 [details] more This is a crazy patch. I sort of went on an optimization walkabout.

Created attachment 303769 [details] the patch

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.

Bots don't look happy.

Created attachment 303773 [details] the patch

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 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.

(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.

Created attachment 303845 [details] the patch

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 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.

(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 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.

Created attachment 303899 [details] the patch

Comment on attachment 303899 [details] the patch r=mlam

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.

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

(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:

(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

(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

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

(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)