Bug 160100 - REGRESSION(203616): no FTL testing was inadvertently removed
Summary: REGRESSION(203616): no FTL testing was inadvertently removed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 160033
  Show dependency treegraph
 
Reported: 2016-07-22 14:23 PDT by Michael Saboff
Modified: 2016-07-22 16:18 PDT (History)
0 users

See Also:


Attachments
Patch for Landing (4.37 KB, patch)
2016-07-22 14:35 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2016-07-22 14:23:17 PDT
The runXXNoFTL tests should not have been removed in change set <http://trac.webkit.org/changeset/203616>.
Comment 1 Michael Saboff 2016-07-22 14:35:41 PDT
Created attachment 284373 [details]
Patch for Landing
Comment 2 Mark Lam 2016-07-22 14:57:16 PDT
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.
Comment 3 Michael Saboff 2016-07-22 15:10:58 PDT
(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.
Comment 4 Michael Saboff 2016-07-22 16:18:13 PDT
Committed r203625: <http://trac.webkit.org/changeset/203625>