Move detection of flaky JSC tests to run-jsc-stress-tests
Created attachment 442909 [details] Patch
Created attachment 443296 [details] Patch
Stephan, the changes in the playstation code are minimal but could still use some review and/or testing.
A quick test with these changes on a subset of a couple hundred tests (with no failures) was okay on device. I'm running a larger (about 8000 test) test set, but it'll likely take a few hours.
Okay - I ran a larger set, then ran a failing test with --filter and --treat-failing-as-flaky=0.6,10,200 but got D:/github/WebKit/Tools/Scripts/webkitruby/jsc-stress-test/executor.rb:48:in `refreshExecution': undefined method `prepareTestRunner' for #<NormalTestsExecutor:0x0000000008b92ca8 @runlist=[#<Plan:0x0000000006f826a8 @directory=#<Pathname:.tests/ChakraCore.yaml/ChakraCore/test/es5>, @arguments=["--useFTLJIT=false", "--useFunctionDotArguments=true", "--validateExceptionChecks=true", "--useDollarVM=true", "--maxPerThreadStackUsage=1572864", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--thresholdForOMGOptimizeAfterWarmUp=20", "--thresholdForOMGOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "--useEagerCodeBlockJettisonTiming=true", "--repatchBufferingCountdown=0", "jsc-lib.js", "hasItem.js"], @family="ChakraCore.yaml/ChakraCore/test/es5/hasItem.js", @name="ChakraCore.yaml/ChakraCore/test/es5/hasItem.js.default", @outputHandler=#<Proc:0x0000000006f82928 D:/github/WebKit/Tools/Scripts/webkitruby/jsc-stress-test/writer-playstation.rb:56>, @errorHandler=#<Proc:0x0000000006f82978 D:/github/WebKit/Tools/Scripts/webkitruby/jsc-stress-test/writer-playstation.rb:161>, @flaky=#<Flaky:0x00000000091bc4d0 @passPercentage=0.6, @maxTries=10>, @additionalEnv=[], @index=47>], @serialPlans=#<Set: {}>, @remoteHosts=nil, @treatFailingAsFlaky=#<OpenStruct passPercentage=0.6, maxTries=10, maxFailing=200>, @infraIterationsFloor=3, @iterationsCeiling=10, @maxIterations=10, @logStream=#<IO:<STDERR>>, @testRunner=#<TestRunnerMake:0x0000000008b92d48 @testRunnerType=:make, @runnerDir=#<Pathname:D:/github/WebKit/WebKitBuild/Release/bin/jsc-stress-results/.runner>>> (NoMethodError) It looks like the prepareTestRunner method was removed in the patch but is still referenced in BaseTestsExecutor's refreshExecution.
Oops, looks like my last refactor to allow keeping state between the prepare*Runner and testRunnerCommand invalidated all my testing. Will have to submit a new patch after trying all combinations with fault injection again. Thanks for taking the time!
<rdar://problem/85099219>
Created attachment 443506 [details] Patch
This should have fixed it. Stephan, if you could try out your larger test set, that'd be great, still need to properly exercise the playstation paths.
It looks like my previous change for the non-find based code path for going through the result files breaks this probably for non-cygwin windows hosts (probably just wincairo and playstation in practice) because the file is open so the unlink in prepareScripts fails. In getStatusMap we're not properly closing the file in that path, there's probably a more idiomatic way, but I was just hacking it to see. - line = File.open(name).first + f = File.open(name) + line = f.first + f.close() With that the test of a single failing test on playstation with a treat-failing-as-flaky did retry successfully. I'm now running the large set, but given the small ones passed, things seem likely to work.
(In reply to Stephan Szabo from comment #10) > It looks like my previous change for the non-find based code path for going > through the result files breaks this probably for non-cygwin windows hosts > (probably just wincairo and playstation in practice) because the file is > open so the unlink in prepareScripts fails. > > In getStatusMap we're not properly closing the file in that path, there's > probably a more idiomatic way, but I was just hacking it to see. > > - line = File.open(name).first > + f = File.open(name) > + line = f.first > + f.close() > > With that the test of a single failing test on playstation with a > treat-failing-as-flaky did retry successfully. Oops. Glad we caught that. FWIW I think a more idiomatic fix would be File.open(name, 'r') { |f| line = f.first processStatusLine(map, "./#{name} #{line}") } I guess it's better if you submit something along those lines as a separate fix? > I'm now running the large set, but given the small ones passed, things seem > likely to work. Thanks, that really helps.
Comment on attachment 443506 [details] Patch Is it possible for us to separate the move parts of this patch from the code changes? Difficult to review the ruby changes since bugzilla isn't representing the move very well.
(In reply to Jonathan Bedard from comment #12) > Comment on attachment 443506 [details] > Patch > > Is it possible for us to separate the move parts of this patch from the code > changes? Difficult to review the ruby changes since bugzilla isn't > representing the move very well. Oops. Yah, I guess I can upload a new patch (though I'll need to repeat some of the testing). FWIW it should look much better if you apply-attachment and use git diff (which does rename detection).
(In reply to Angelos Oikonomopoulos from comment #13) > (In reply to Jonathan Bedard from comment #12) > > Comment on attachment 443506 [details] > > Patch > > > > Is it possible for us to separate the move parts of this patch from the code > > changes? Difficult to review the ruby changes since bugzilla isn't > > representing the move very well. > > Oops. Yah, I guess I can upload a new patch (though I'll need to repeat some > of the testing). FWIW it should look much better if you apply-attachment and > use git diff (which does rename detection). Other possibility: although we don't have EWS hooked up yet, `git-webkit pr` will create a GitHub pull-request, and the diff might be easier to read there.
Created attachment 443557 [details] Patch
New patch skips the file renaming.
As a note on earlier patch version plus changed mention, the playstation run of the about 8000 tests worked fine. Looking to do a version of the unlink fix now. (Sorry, semi-off right now, so response time's a bit slow)
Submitted https://bugs.webkit.org/show_bug.cgi?id=232851 for result file handling update.
Comment on attachment 443557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443557&action=review The buildbot and run-javascriptcore-tests bits look good, I'm not familiar with the architecture of our stress tests or Ruby, though > Tools/CISupport/ews-build/steps.py:1887 > + AnalyzeJSCTestsResults()]) We need to coordinate landing this change with an EWS restart. > Tools/Scripts/run-javascriptcore-tests:287 > + --treat-failing-as-flaky Treat failing stress tests as flaky This option is quite a bit more complicated than the help message indicates. I would like something like what you outline in the Changelog about passPercentage,maxTries,maxFailing
Created attachment 443676 [details] Patch
(In reply to Jonathan Bedard from comment #19) > Comment on attachment 443557 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443557&action=review > > The buildbot and run-javascriptcore-tests bits look good, I'm not familiar > with the architecture of our stress tests or Ruby, though Yah, according to git blame only a few people have touched these parts of the file in the last few years. > > Tools/CISupport/ews-build/steps.py:1887 > > + AnalyzeJSCTestsResults()]) > > We need to coordinate landing this change with an EWS restart. Possibly, though AFAIU things /should/ work fine until the EWS restart. The EWS buildbots will not be passing in `--treat-failing-as-flaky`, so the script's behavior should be exactly the same. > > Tools/Scripts/run-javascriptcore-tests:287 > > + --treat-failing-as-flaky Treat failing stress tests as flaky > > This option is quite a bit more complicated than the help message indicates. > I would like something like what you outline in the Changelog about > passPercentage,maxTries,maxFailing A very valid point. Uploaded a new version of the patch with an extended help message for the option.
(In reply to Angelos Oikonomopoulos from comment #21) > (In reply to Jonathan Bedard from comment #19) > > Comment on attachment 443557 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=443557&action=review > > > > The buildbot and run-javascriptcore-tests bits look good, I'm not familiar > > with the architecture of our stress tests or Ruby, though > > Yah, according to git blame only a few people have touched these parts of > the file in the last few years. CC'ing msaboff and fpizlo for the run-jsc-stress-tests parts.
Ping. Would be good to improve the flaky handling for JSC.
Calling all rubyists.
Comment on attachment 443676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443676&action=review I touched base with Angelos about this patch today. With most Apple folks out next week and no JSC folks commenting, I don't think it makes sense to continue blocking this patch. I'm not a Ruby expert, but looking at this patch, combined with EWS and the fact that it shouldn't change behavior until builedbot is deployed, I'm r+ing with the expectation that Angelos will land Monday, Nov 22nd and handle any fallout next week. > Tools/Scripts/run-jsc-stress-tests:620 > + @flaky = flaky This name doesn't indicate well what expect "flaky" to be. I'm assuming it's the Flaky object? Might be good to indicate that in the initializer by calling it flakyObject. I think it's fine to keep "flaky" as is in the actual class. > Tools/Scripts/run-jsc-stress-tests:845 > +class Flaky Flaky seems like the wrong name for this object. It looks to like it's more accurately "Tolerance" or "RetryParameters". If we rename it, we should also rename any member variables which contain it.
Created attachment 444921 [details] Patch
Created attachment 444922 [details] Patch
(In reply to Jonathan Bedard from comment #25) [...] > > Tools/Scripts/run-jsc-stress-tests:620 > > + @flaky = flaky > > This name doesn't indicate well what expect "flaky" to be. I'm assuming it's > the Flaky object? Might be good to indicate that in the initializer by > calling it flakyObject. I think it's fine to keep "flaky" as is in the > actual class. > > > Tools/Scripts/run-jsc-stress-tests:845 > > +class Flaky > > Flaky seems like the wrong name for this object. It looks to like it's more > accurately "Tolerance" or "RetryParameters". If we rename it, we should also > rename any member variables which contain it. RetryParameters is much more accurate. Updated the patch and added comments to explain that Plans that have @retryParameters set are treated as flaky.
Committed r286109 (244496@main): <https://commits.webkit.org/244496@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 444922 [details].