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.
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.