Summary: | Don't run FTL related JSC stress tests on non-FTL platforms | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||||
Component: | Tools / Tests | Assignee: | Csaba Osztrogonác <ossy> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Blocker | CC: | bfulgham, fpizlo, ggaren, lforschler, mark.lam, msaboff, ossy, saam, ysuzuki | ||||||||||
Priority: | P1 | ||||||||||||
Version: | Other | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 160021, 160100 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Csaba Osztrogonác
2016-07-21 11:01:47 PDT
Created attachment 284231 [details]
patch
*** Bug 160058 has been marked as a duplicate of this bug. *** 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 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.
(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. 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.
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.
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 ... (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? (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. I'm testing a patch now and will post soon. (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. (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. :) Created attachment 284355 [details]
Updated patch
Comment on attachment 284355 [details]
Updated patch
r=me if bots are green.
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? (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. (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. Committed r203616: <http://trac.webkit.org/changeset/203616> (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. |