Bug 160100 – REGRESSION(203616): no FTL testing was inadvertently removed

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

The runXXNoFTL tests should not have been removed in change set <http://trac.webkit.org/changeset/203616>.

[Michael Saboff]

Created attachment 284373 [details]
Patch for Landing

[Mark Lam]

Comment on attachment 284373 [details]
Patch for Landing

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

> Tools/Scripts/run-jsc-stress-tests:942
> +        runNoFTL if $isFTLPlatform
>          runFTLNoCJITValidate if $isFTLPlatform

Since you have more than 1 thing to run if $isFTLPlatform, why not apply the same idiom of returning early if !$isFTLPlatform as is done in other run configurations?

> Tools/Scripts/run-jsc-stress-tests:1201
> +            runLayoutTestNoFTL

Do we already disable the FTL by default?  If not, you should explicitly disable the FTL in the definition of runLayoutTestNoFTL.

> Tools/Scripts/run-jsc-stress-tests:1205
> +            noFTLRunLayoutTest

Is noFTLRunLayoutTest different than runLayoutTestNoFTL?  If they are the same, we should get rid of one.  If they are different, don't we want to run runLayoutTestNoFTL as well if $isFTLPlatform (because we want that coverage)?

> Tools/Scripts/run-jsc-stress-tests:1364
> +    runNoisyTest("no-ftl")

Ditto.  Is the FTL already disabled by default?  If not, we should explicitly disable it.

[Michael Saboff]

(In reply to comment #2)
> Comment on attachment 284373 [details]
> Patch for Landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284373&action=review
> 
> > Tools/Scripts/run-jsc-stress-tests:942
> > +        runNoFTL if $isFTLPlatform
> >          runFTLNoCJITValidate if $isFTLPlatform
> 
> Since you have more than 1 thing to run if $isFTLPlatform, why not apply the
> same idiom of returning early if !$isFTLPlatform as is done in other run
> configurations?

Done.

> > Tools/Scripts/run-jsc-stress-tests:1201
> > +            runLayoutTestNoFTL
> 
> Do we already disable the FTL by default?  If not, you should explicitly
> disable the FTL in the definition of runLayoutTestNoFTL.

FTL is disabled by default.

> > Tools/Scripts/run-jsc-stress-tests:1205
> > +            noFTLRunLayoutTest
> 
> Is noFTLRunLayoutTest different than runLayoutTestNoFTL?  If they are the
> same, we should get rid of one.  If they are different, don't we want to run
> runLayoutTestNoFTL as well if $isFTLPlatform (because we want that coverage)?

They are different.  noFTLRunLayoutTest is the DFG only equivalent of the three test variants we run if FTL is enabled.  Since this is a "quick" test only one flavor is needed.

> > Tools/Scripts/run-jsc-stress-tests:1364
> > +    runNoisyTest("no-ftl")
> 
> Ditto.  Is the FTL already disabled by default?  If not, we should
> explicitly disable it.

See above.

[Michael Saboff]

Committed r203625: <http://trac.webkit.org/changeset/203625>