Bug 200789 - mozilla-tests.yaml/js1_5/Array/regress-101964.js is frequently failing on JSC EWS bots
Summary: mozilla-tests.yaml/js1_5/Array/regress-101964.js is frequently failing on JSC...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
: 204406 204502 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-08-15 14:08 PDT by Ryan Haddad
Modified: 2019-12-08 06:31 PST (History)
15 users (show)

See Also:


Attachments
Patch (1.35 KB, patch)
2019-12-02 09:45 PST, Keith Miller
mark.lam: review+
Details | Formatted Diff | Diff
proposed patch. (5.22 KB, patch)
2019-12-02 12:18 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (3.54 KB, patch)
2019-12-02 12:21 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Radar WebKit Bug Importer 2019-08-15 14:09:06 PDT
<rdar://problem/54361916>
Comment 2 Alexey Proskuryakov 2019-10-23 09:28:48 PDT
There were 15 false positives because of this on EWS in a month, so worth looking into soon.
Comment 3 Alexey Proskuryakov 2019-11-22 09:21:31 PST
*** Bug 204502 has been marked as a duplicate of this bug. ***
Comment 4 Alexey Proskuryakov 2019-11-22 09:21:41 PST
*** Bug 204406 has been marked as a duplicate of this bug. ***
Comment 5 Keith Miller 2019-11-22 10:00:12 PST
Let's just delete that test... I'm assuming it's failing because it's being run with dozens of other processes so it happens to get scheduled weirdly.
Comment 6 Alexey Proskuryakov 2019-11-22 10:11:02 PST
Indeed! Testing that something takes under 100 ms makes no sense when running many tests in parallel.

mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline: STATUS: Performance: truncating even very large arrays should be fast!
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline: FAILED!: [reported from test()] Section 1 of test -
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline: FAILED!: [reported from test()] Expected value 'Truncation took less than 100 ms', Actual value 'Truncation took 193 ms'
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline: FAILED!: [reported from test()]
Comment 7 Aakash Jain 2019-11-28 05:06:52 PST
This test flaked again in https://ews-build.webkit.org/#/builders/26/builds/469


mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases: Detected failures:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases: BUGNUMBER: 101964
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases: STATUS: Performance: truncating even very large arrays should be fast!
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases: FAILED!: [reported from test()] Section 1 of test -
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases: FAILED!: [reported from test()] Expected value 'Truncation took less than 100 ms', Actual value 'Truncation took 336 ms'


Note that these tests are running on Raspberry Pi on Igalia JSC queues (ARMv7 and MIPSEL).
Comment 8 Aakash Jain 2019-11-28 11:49:15 PST
Flaked again in: https://ews-build.webkit.org/#/builders/26/builds/527
Comment 9 Paulo Matos 2019-12-02 07:01:37 PST
I have noticed, as Aakash that this test keeps passing/failing on mips and armv7. I have opened bug 204746 to skip it on these architectures, but I am happy if it's removed altogether.
Comment 10 Guillaume Emont 2019-12-02 09:30:48 PST
As Paulo just said, it flakes quite regularly on mips (though our mips test devices are not raspberry pis, obviously, but they're quite slow ci20 dev boards) and  on armv7 (rpi3s). Running the test on its own with nothing else running on the board it always passes though, failures only seem to happen while running other tests.

Maybe raising the timeout could be a solution? I think on mips when it fails, it's still always under 200ms, I guess we could do 300ms to be on the safe side, and from what I understand, if the regression it's testing would happen, the truncation would take an order of magnitude more than that, so a timeout of 300-500ms might be more appropriate for this test?
Comment 11 Aakash Jain 2019-12-02 09:38:59 PST
(In reply to Guillaume Emont from comment #10)
> Maybe raising the timeout could be a solution? I think on mips when it
> fails, it's still always under 200ms, I guess we could do 300ms to be on the safe side
On armv7 it took 336ms in https://ews-build.webkit.org/#/builders/26/builds/469/steps/9/logs/stdio (line 37271). So, for armv7 we would either need to skip/disable this test or choose higher timeout.
Comment 12 Keith Miller 2019-12-02 09:41:01 PST
I think we should just get rid of this test. We don't run tests in isolation so even on high power machines the system could and likely does suspend this test to run other tests.
Comment 13 Keith Miller 2019-12-02 09:45:47 PST
Created attachment 384634 [details]
Patch
Comment 15 Mark Lam 2019-12-02 09:52:49 PST
(In reply to Aakash Jain from comment #14)
> Should we delete
> https://trac.webkit.org/browser/webkit/trunk/JSTests/mozilla/js1_5/Array/
> regress-101964.js as well?

I was just looking at the test.  It seems strange that it should ever time out.  I suggest filing a bug to investigate why it times out.
Comment 16 Mark Lam 2019-12-02 09:55:44 PST
(In reply to Mark Lam from comment #15)
> (In reply to Aakash Jain from comment #14)
> > Should we delete
> > https://trac.webkit.org/browser/webkit/trunk/JSTests/mozilla/js1_5/Array/
> > regress-101964.js as well?
> 
> I was just looking at the test.  It seems strange that it should ever time
> out.  I suggest filing a bug to investigate why it times out.

Sorry.  I mean I don't see why the test should ever fail.  It's testing a trivial operation.  I think we should do some due dilligence to make sure there's not some latent perf bug here.
Comment 17 Keith Miller 2019-12-02 10:14:28 PST
(In reply to Mark Lam from comment #16)
> (In reply to Mark Lam from comment #15)
> > (In reply to Aakash Jain from comment #14)
> > > Should we delete
> > > https://trac.webkit.org/browser/webkit/trunk/JSTests/mozilla/js1_5/Array/
> > > regress-101964.js as well?
> > 
> > I was just looking at the test.  It seems strange that it should ever time
> > out.  I suggest filing a bug to investigate why it times out.
> 
> Sorry.  I mean I don't see why the test should ever fail.  It's testing a
> trivial operation.  I think we should do some due dilligence to make sure
> there's not some latent perf bug here.

It could fail because the os schedules so other tests to run between the time checks.
Comment 18 Ross Kirsling 2019-12-02 10:21:23 PST
Interestingly, we already raised the threshold from 50ms to 100ms three years ago:
https://github.com/WebKit/webkit/commit/50c9e3155e5fa74b2579f8240fd00afbda4a00ae
Comment 19 Mark Lam 2019-12-02 11:32:52 PST
(In reply to Keith Miller from comment #17)
> (In reply to Mark Lam from comment #16)
> > (In reply to Mark Lam from comment #15)
> > > (In reply to Aakash Jain from comment #14)
> > > > Should we delete
> > > > https://trac.webkit.org/browser/webkit/trunk/JSTests/mozilla/js1_5/Array/
> > > > regress-101964.js as well?
> > > 
> > > I was just looking at the test.  It seems strange that it should ever time
> > > out.  I suggest filing a bug to investigate why it times out.
> > 
> > Sorry.  I mean I don't see why the test should ever fail.  It's testing a
> > trivial operation.  I think we should do some due dilligence to make sure
> > there's not some latent perf bug here.
> 
> It could fail because the os schedules so other tests to run between the
> time checks.

If this is the case, then we can trivially solve this by changing the test to use CPU time instead of wall clock time.  That should also prove that there's nothing else wrong.  I'll prepare a patch.
Comment 20 Mark Lam 2019-12-02 12:18:56 PST
Created attachment 384646 [details]
proposed patch.
Comment 21 Mark Lam 2019-12-02 12:21:39 PST
Created attachment 384647 [details]
proposed patch.
Comment 22 Keith Miller 2019-12-02 12:24:08 PST
Comment on attachment 384647 [details]
proposed patch.

r=me.
Comment 23 Mark Lam 2019-12-02 13:32:58 PST
Comment on attachment 384647 [details]
proposed patch.

Thanks for the review.  Landing now.
Comment 24 WebKit Commit Bot 2019-12-02 14:16:49 PST
Comment on attachment 384647 [details]
proposed patch.

Clearing flags on attachment: 384647

Committed r253008: <https://trac.webkit.org/changeset/253008>
Comment 25 WebKit Commit Bot 2019-12-02 14:16:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Guillaume Emont 2019-12-08 06:31:39 PST
(In reply to Mark Lam from comment #19)
> (In reply to Keith Miller from comment #17)
> > 
> > It could fail because the os schedules so other tests to run between the
> > time checks.
> 
> If this is the case, then we can trivially solve this by changing the test
> to use CPU time instead of wall clock time.  That should also prove that
> there's nothing else wrong.  I'll prepare a patch.

Looking at the mips bot results, this seems to have solved it: the test lately was failing every second run, and it's now been 16 runs without it failing.
Good idea, thanks!