Bug 160033 – Don't run FTL related JSC stress tests on non-FTL platforms

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

https://trac.webkit.org/changeset/203440 removed the possibility
to skip redundant tests on non FTL platforms which made testing
terrible slow ( just to be able to paint some bots green ... )

We don't want to run 36k tests instead of 20k tests when 16k
are redundant and don't add anything for test coverage. We
don't want 50% extra test runtime just for fun.

There are 9 active tester buildbot on https://build.webkit.org/waterfall
which suffer from the intrusive change intruduced in r203440 :
(the 3 x86 bots ran redundant tests previously because of another bug )

- There is no FTL-JIT on 32 bit platforms (x86, ARMv7) and there won't be ever.
  - Apple El Capitan 32-bit JSC (BuildAndTest)
  - Apple Yosemite 32-bit JSC (BuildAndTest)
  - Apple Win 7 Release (Tests)
  - Apple Win 7 Debug (Tests)
  - GTK Linux 32-bit Release
  - JSCOnly Linux ARMv7 Thumb2 Release
  - JSCOnly Linux ARMv7 Traditional Release

- FTL-JIT is ready on 64-bit Windows (bug145366), but review process is very slow.
  - no tester bot

- FTL-JIT builds and works more or less on AArch64 Linux (bug144039), but there are 
many crashes on it and nobody is interested in fixing these crashes, so it won't
be enabled in the near future.
  - JSCOnly Linux AArch64 Release


I proposed some renaming in bug160021 to make what Filip wanted originally in r203440.
After bug160021 default will really mean default, it is just a renaming.

Based on bug160021 I'm going to add similar logic to RJST script as we had previously.
My idea is to skip all tests on non-FTL platforms as we skipped previously, but we
don't need any command line option, RJST can determine x86,ARM,Windows platforms
from JSC binary to be able to skip redundant tests.

I don't want to solve all problems now in this bug, it is just firefighting
to fix the 9 bots/platforms above. Later we can refactor test modes for the
real life in present, for example remove --useFTLJIT=false from BASE_OPTIONS
and add it explicitly to modes where we really want to disable FTL JIT.

[Csaba Osztrogonác]

Created attachment 284231 [details]
patch

[Csaba Osztrogonác]

*** Bug 160058 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

Dear JSC reviewers, please review this patch to unbroke JSC testing
on non-FTL bots, eg: Apple internal ARM bots, Apple Windows bots,
Linux ARM bots

[Michael Saboff]

Comment on attachment 284231 [details]
patch

r-

I think this change is more complicated than it needs to be.  For example there is no need to make the runXXX("no-ftl") calls conditional.  Instead of making each FTL related run("...") conditional, only invoke the FTL runFTLxxx methods on FTL platforms.  runDefault would needs some work, either splitting or making passing FTL_OPTIONS conditional.

Also, this doesn't fix the issue for arm64.

[Csaba Osztrogonác]

(In reply to comment #4)
> Comment on attachment 284231 [details]
> patch
> 
> r-
> 
> I think this change is more complicated than it needs to be.  For example
> there is no need to make the runXXX("no-ftl") calls conditional.  Instead of
> making each FTL related run("...") conditional, only invoke the FTL
> runFTLxxx methods on FTL platforms. 

runXXXNoFTL == runXXXDefault on non FTL platforms. We should skip
one of them to avoid redundancy. The intent of r203440 was not
skip the default one, because it is confusing. And the non-FTL
platforms would skip the 3 tests marked to run only default config.

There are more call site for runFTLxxx functions then functions.
I intentionally didn't want to change the original behavior,
that's why I proposed more or less the pre-r203440 state. Of
course I can refactor the patch, as you wanted.
 
> Also, this doesn't fix the issue for arm64.
Good catch, I will add it to the list.

> runDefault would needs some work, either splitting 
> or making passing FTL_OPTIONS conditional.

Passing useFTLJIT=true/false on non-FTL platform doesn't do anything.
We can pass these options unconditionally, but won't have any effect.

[Csaba Osztrogonác]

Created attachment 284342 [details]
Patch

I started to refactor how you suggested. But it seems terrible and it's so easy to make a mistake and run redundant tests. Not to mention if somebody add a //@ runFTLEager to the test directly. I still say I don't want to fix everything in this patch. I only want to have working bots without useless test running before Filip's patch.

[Csaba Osztrogonác]

Created attachment 284346 [details]
Patch

WIP patch. Because stress,mozilla-tests.yaml,regress and jsc-layout-tests.yaml tests are the 95% of all tests, it's more than enough to skip redundant tests from them. I don't care with exotic configs which covers 5-10-50 tests, we can live redundant test cases in these test groups.

[Csaba Osztrogonác]

If we won't agree how to fix the mess after Filip's "cleanup"
in some days, I will apply my original patch --locally--  on 
JSCOnly bots and don't care how do you want to fix it properly.
I won't waste my time to fix what worked properly previously ...

[Mark Lam]

(In reply to comment #8)
> If we won't agree how to fix the mess after Filip's "cleanup"
> in some days, I will apply my original patch --locally--  on 
> JSCOnly bots and don't care how do you want to fix it properly.
> I won't waste my time to fix what worked properly previously ...

Hi Ossy, I see 3 patches.  Are 2 of them obsolete?

[Csaba Osztrogonác]

(In reply to comment #9)
> Hi Ossy, I see 3 patches.  Are 2 of them obsolete?

The 1st one was r- -ed by Mark, then tried to do what Mark suggested,
but I think it would be terrible and error prone (2nd patch).

And then started to try a minimal patch to concentrate the majority
of the tests without caring about exotic test groups. It is the 3rd
one, but it's not tested at all yet.

I'm really confused now, because RJST test groups are giant hacks
and I wanted to avoid fixing everything in this bug. And I don't
and won't have time for it at all. My only one goal is to get back
the possibility to skip redundant tests on non FTL platfors, as much
as we can easily.

[Michael Saboff]

I'm testing a patch now and will post soon.

[Mark Lam]

(In reply to comment #10)
> (In reply to comment #9)
> > Hi Ossy, I see 3 patches.  Are 2 of them obsolete?
> 
> The 1st one was r- -ed by Mark, then tried to do what Mark suggested,
> but I think it would be terrible and error prone (2nd patch).

I think you meant Michael.

[Csaba Osztrogonác]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Hi Ossy, I see 3 patches.  Are 2 of them obsolete?
> > 
> > The 1st one was r- -ed by Mark, then tried to do what Mark suggested,
> > but I think it would be terrible and error prone (2nd patch).
> 
> I think you meant Michael.

Yes, of course. :)

[Michael Saboff]

Created attachment 284355 [details]
Updated patch

[Mark Lam]

Comment on attachment 284355 [details]
Updated patch

r=me if bots are green.

[Filip Pizlo]

Comment on attachment 284355 [details]
Updated patch

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

> Tools/Scripts/run-jsc-stress-tests:-903
> -        runNoFTL
> +        runDefault
>          runAlwaysTriggerCopyPhase
>          if $jitTests
>              runNoLLInt
>              runNoCJITValidatePhases
>              runDFGEager
>              runDFGEagerNoCJITValidate
> -            runDefault
> +            runDFGMaximalFlushPhase
> +
> +            return if !$isFTLPlatform
> +
>              runFTLNoCJITValidate
>              runFTLNoCJITNoPutStackValidate
>              runFTLNoCJITNoInlineValidate
>              runFTLEager
>              runFTLEagerNoCJITValidate
>              runFTLNoCJITSmallPool
> -            runDFGMaximalFlushPhase

Does this mean that we never do runNoFTL when FTL is enabled?  Is that intentional?

[Michael Saboff]

(In reply to comment #16)
> Comment on attachment 284355 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284355&action=review
> 
> > Tools/Scripts/run-jsc-stress-tests:-903
> > -        runNoFTL
> > +        runDefault
> >          runAlwaysTriggerCopyPhase
> >          if $jitTests
> >              runNoLLInt
> >              runNoCJITValidatePhases
> >              runDFGEager
> >              runDFGEagerNoCJITValidate
> > -            runDefault
> > +            runDFGMaximalFlushPhase
> > +
> > +            return if !$isFTLPlatform
> > +
> >              runFTLNoCJITValidate
> >              runFTLNoCJITNoPutStackValidate
> >              runFTLNoCJITNoInlineValidate
> >              runFTLEager
> >              runFTLEagerNoCJITValidate
> >              runFTLNoCJITSmallPool
> > -            runDFGMaximalFlushPhase
> 
> Does this mean that we never do runNoFTL when FTL is enabled?  Is that
> intentional?

runDefault is that same as runNoFTL with the addition of FTL_OPTIONS.  Therefore we only want one of those.  For platforms with the FTL enabled, we want to use FTL_OPTIONS.  For platforms without the FTL enabled, the FTL_OPTIONS are no-ops.

[Mark Lam]

(In reply to comment #17)
> (In reply to comment #16)
> > Comment on attachment 284355 [details]
> > Updated patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=284355&action=review
> > 
> > > Tools/Scripts/run-jsc-stress-tests:-903
> > > -        runNoFTL
> > > +        runDefault
> > >          runAlwaysTriggerCopyPhase
> > >          if $jitTests
> > >              runNoLLInt
> > >              runNoCJITValidatePhases
> > >              runDFGEager
> > >              runDFGEagerNoCJITValidate
> > > -            runDefault
> > > +            runDFGMaximalFlushPhase
> > > +
> > > +            return if !$isFTLPlatform
> > > +
> > >              runFTLNoCJITValidate
> > >              runFTLNoCJITNoPutStackValidate
> > >              runFTLNoCJITNoInlineValidate
> > >              runFTLEager
> > >              runFTLEagerNoCJITValidate
> > >              runFTLNoCJITSmallPool
> > > -            runDFGMaximalFlushPhase
> > 
> > Does this mean that we never do runNoFTL when FTL is enabled?  Is that
> > intentional?
> 
> runDefault is that same as runNoFTL with the addition of FTL_OPTIONS. 
> Therefore we only want one of those.  For platforms with the FTL enabled, we
> want to use FTL_OPTIONS.  For platforms without the FTL enabled, the
> FTL_OPTIONS are no-ops.

I think Fil meant that on platforms with FTL enabled, we also want to test the no FTL variants.

[Michael Saboff]

Committed r203616: <http://trac.webkit.org/changeset/203616>

[Michael Saboff]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Comment on attachment 284355 [details]
> > > Updated patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=284355&action=review
> > > 
> > > > Tools/Scripts/run-jsc-stress-tests:-903
> > > > -        runNoFTL
> > > > +        runDefault
> > > >          runAlwaysTriggerCopyPhase
> > > >          if $jitTests
> > > >              runNoLLInt
> > > >              runNoCJITValidatePhases
> > > >              runDFGEager
> > > >              runDFGEagerNoCJITValidate
> > > > -            runDefault
> > > > +            runDFGMaximalFlushPhase
> > > > +
> > > > +            return if !$isFTLPlatform
> > > > +
> > > >              runFTLNoCJITValidate
> > > >              runFTLNoCJITNoPutStackValidate
> > > >              runFTLNoCJITNoInlineValidate
> > > >              runFTLEager
> > > >              runFTLEagerNoCJITValidate
> > > >              runFTLNoCJITSmallPool
> > > > -            runDFGMaximalFlushPhase
> > > 
> > > Does this mean that we never do runNoFTL when FTL is enabled?  Is that
> > > intentional?
> > 
> > runDefault is that same as runNoFTL with the addition of FTL_OPTIONS. 
> > Therefore we only want one of those.  For platforms with the FTL enabled, we
> > want to use FTL_OPTIONS.  For platforms without the FTL enabled, the
> > FTL_OPTIONS are no-ops.
> 
> I think Fil meant that on platforms with FTL enabled, we also want to test
> the no FTL variants.

Added <https://bugs.webkit.org/show_bug.cgi?id=160100> to add back in the no FTL variants.