Bug 169510 - REGRESSION(r213645): It made JSC tests super slow and timeout on AArch64 Linux
Summary: REGRESSION(r213645): It made JSC tests super slow and timeout on AArch64 Linux
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All Linux
: P1 Blocker
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 108645 169300
  Show dependency treegraph
 
Reported: 2017-03-11 00:46 PST by Csaba Osztrogonác
Modified: 2017-04-11 01:27 PDT (History)
11 users (show)

See Also:


Attachments
Patch (2.31 KB, patch)
2017-04-03 12:03 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2017-03-11 00:46:59 PST
before r213645: 
- https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/4895
- runtime: 1h 15mins
- only 7 failures

after r213645: 
- https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/4950
- runtime: 10h 30 mins (stopped after it, full test runtime would have taken days)
- zillion timeouts

Note, that https://trac.webkit.org/changeset/213645 broke the build too
which was fixed in https://trac.webkit.org/changeset/213705. But I tested
r213645 with the buildfix directly and it showed that r213645 caused this
serious regression.

Is there any way to disable this super slow new feature on Linux?
I don't and won't have to debug what caused this regeression in your patch.
Comment 1 Csaba Osztrogonác 2017-03-11 05:58:11 PST
I disabled it on Linux in https://trac.webkit.org/changeset/213758.

Please enable it again once somebody fixed this regression.
Comment 2 Filip Pizlo 2017-03-11 07:44:50 PST
We should figure out what's going on here. This is probably not a Linux versus Darwin thing but a CPU model thing.

Here's a possible hint: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150921/300953.html

It looks like in LLVM, they have to emit clrex when exiting ldx/stx (LL/SC) loops because "some uarchs" need it for "performance". I tried emitting clrex, and on my hardware, it made no difference.

I think in our jargon, "clrex" should be called "WTF::clearLink()", since we use the old computer science terminology for this (load link, store conditional) rather than the ARM terminology (reservations).  The way this would look like is that you'd want Atomic<>::transaction() to call clearLink() whenever it returns after having done a loadLink() but before the storeCond() - basically anytime that we would otherwise be leaving the link (the reservation) in place.

If this works then just as transaction() abstracts LL/SC and CAS behind prepare/attempt, you probably want a giveUp() method that is a wrapper for clearLink() on LL_SC platforms and a no-op on CAS platforms. You probably want clearLink() to only be declared when HAVE(LL_SC) just as loadLink()/storeCond() are only declared when HAVE(LL_SC).

I think it's OK to do clearLink() on all ARM64s, if it helps your perf.

If this works, then can you also put a FIXME in B3LowerToAir's code that emits loadlink/storecond, and MacroAssembler's loadLink/storeCond code, and link it to a bug?  Once I get around to implementing SharedArrayBuffer atomics, I'll definitely need to use MacroAssembler's and B3's LL/SC. It won't be possible to easily get around this.
Comment 3 Csaba Osztrogonác 2017-03-13 05:21:56 PDT
(In reply to comment #2)
> We should figure out what's going on here. This is probably not a Linux
> versus Darwin thing but a CPU model thing.
...

Of course, yes and thanks for the detailed info. But as I said 
I don't and won't have to do it myself in the (near) future.

If somebody else is interested in debugging this issue,
feel free to pick it up.
Comment 4 Zan Dobersek 2017-04-03 12:03:42 PDT
Created attachment 306092 [details]
Patch
Comment 5 Build Bot 2017-04-03 12:05:00 PDT
Attachment 306092 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:193:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/Atomics.h:213:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Saam Barati 2017-04-10 19:23:35 PDT
Comment on attachment 306092 [details]
Patch

r=me
Comment 7 Zan Dobersek 2017-04-11 01:27:32 PDT
Comment on attachment 306092 [details]
Patch

Clearing flags on attachment: 306092

Committed r215220: <http://trac.webkit.org/changeset/215220>
Comment 8 Zan Dobersek 2017-04-11 01:27:36 PDT
All reviewed patches have been landed.  Closing bug.