The following layout test is flaky on Mojave Debug WK1 imported/blink/storage/indexeddb/blob-basics-metadata.html Probable cause: Unknown, this issue first appeared on this build: https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK1%20(Tests)/builds/3751 Possibly 246039 Flakiness Dashboard: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fblink%2Fstorage%2Findexeddb%2Fblob-basics-metadata.html These was a pass at 29 seconds, possibly marking this test as slow will let it pass.
I cannot reproduce this locally, I am getting instant passes.
storage/indexeddb/modern/objectstore-autoincrement-types.html got slow at the same time. Looking at the bots on the queue, it looks like this started after testing was interrupted, which likely means that we installed a new OS, or maybe just rebooted. Ryan, do you remember what happened to these bots on June 3rd? PASS 3745 r246034 bot183 3746 r246035 bot183 Test interrupted: 3747 3748 FAIL 3749 r246036 bot184 3750 r246037 bot183 3751 r246038 bot184
Actually, I suspect that this could be similar to rdar://problem/50948871. Let's discuss in e-mail if mitigations for that radar work here too.
<rdar://problem/52967035>
*** Bug 200002 has been marked as a duplicate of this bug. ***
According to Alexey: "We probably just need to add opt out in JSC, disabling SMT mitigations on tests (that would be in JSC and webkitpy code, not in machine configuration)." I've added this to run-javascriptcore-tests as well as WebKitTestRunner and DumpRenderTree.
Created attachment 385052 [details] Patch
Comment on attachment 385052 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385052&action=review > Source/JavaScriptCore/runtime/JSCConfig.h:55 > + setenv("JSC_useKernTCSM", "false", 0); This doesn't compile on Windows. But also, is it right to do this in configureForTesting? We certainly don't want to disable this for performance tests, for example. This function name is so generic that I can't tell.
(In reply to Alexey Proskuryakov from comment #8) > Comment on attachment 385052 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385052&action=review > > > Source/JavaScriptCore/runtime/JSCConfig.h:55 > > + setenv("JSC_useKernTCSM", "false", 0); > > This doesn't compile on Windows. > > But also, is it right to do this in configureForTesting? We certainly don't > want to disable this for performance tests, for example. This function name > is so generic that I can't tell. I think this is the right place to do it, but we might want some JSC folks to verify. As far as I can tell, this function is used in a few API tests along with DumpRenderTree and some of the JSC test binaries. (I should actually remove the environment variable in run-javascriptcore-tests) Since we tend to use production binaries to do performance testing, I don't believe it will be run during performance testing, but if we have any performance tests which use DumpRenderTree or WebKitTestRunner as their harness. We could also move these to the harness script entirely, but (as previously mentioned) that wouldn't apply to manual test runs.
Comment on attachment 385052 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385052&action=review >>> Source/JavaScriptCore/runtime/JSCConfig.h:55 >>> + setenv("JSC_useKernTCSM", "false", 0); >> >> This doesn't compile on Windows. >> >> But also, is it right to do this in configureForTesting? We certainly don't want to disable this for performance tests, for example. This function name is so generic that I can't tell. > > I think this is the right place to do it, but we might want some JSC folks to verify. > > As far as I can tell, this function is used in a few API tests along with DumpRenderTree and some of the JSC test binaries. (I should actually remove the environment variable in run-javascriptcore-tests) Since we tend to use production binaries to do performance testing, I don't believe it will be run during performance testing, but if we have any performance tests which use DumpRenderTree or WebKitTestRunner as their harness. > > We could also move these to the harness script entirely, but (as previously mentioned) that wouldn't apply to manual test runs. I think this is not necessary. It is also hacky to set env vars inside JavaScriptCore. > Tools/Scripts/run-javascriptcore-tests:492 > +if (isAppleMacWebKit() || isEmbeddedWebKit()) { > + $envVars .= " JSC_useKernTCSM=false"; > +} I think this should be sufficient for JSC tests. > Tools/WebKitTestRunner/ios/TestControllerIOS.mm:109 > + setenv("JSC_useKernTCSM", "false", 0); > + setenv("__XPC_JSC_useKernTCSM", "false", 0); This is also hacky. Can you set this in the driver script instead? > Tools/WebKitTestRunner/mac/TestControllerMac.mm:93 > + setenv("JSC_useKernTCSM", "false", 0); > + setenv("__XPC_JSC_useKernTCSM", "false", 0); Ditto.
Created attachment 385171 [details] Patch
Comment on attachment 385171 [details] Patch LGTM, but perhaps another reviewer who knows the test scripts better should take a look as well.
Comment on attachment 385171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385171&action=review > Tools/Scripts/run-javascriptcore-tests:491 > + $envVars .= " JSC_useKernTCSM=false"; Why? Don’t we want our testing to test this?
(In reply to Saam Barati from comment #13) > Comment on attachment 385171 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385171&action=review > > > Tools/Scripts/run-javascriptcore-tests:491 > > + $envVars .= " JSC_useKernTCSM=false"; > > Why? Don’t we want our testing to test this? I think it debatable if we want this variable set. We definitely need it for layout tests, since we have so many processes running at once, it's about a 10% performance improvement on test runtime, last I checked. I know Alexey thought that JSC correctness tests could benefit from the gains, but I don't actually know if they're faster in practice.
> Why? Don’t we want our testing to test this? When this was discussed a few months back, pretty sure the consensus was that we want to run the tests quickly, so we need to disable the feature. As for setting it in the binary - this is desirable because the fewer differences there are between running tests in the harness and without the harness, the better.
(In reply to Alexey Proskuryakov from comment #15) > > Why? Don’t we want our testing to test this? > > When this was discussed a few months back, pretty sure the consensus was > that we want to run the tests quickly, so we need to disable the feature. > > As for setting it in the binary - this is desirable because the fewer > differences there are between running tests in the harness and without the > harness, the better. Maybe we should land it in the scripts first and then have a separate discussion about if we want it in the binaries. There are arguments both ways, but I don't think anyone has said we shouldn't be doing this in automation.
marked storage/indexeddb/modern/objectstore-autoincrement-types.html as timing out in https://trac.webkit.org/changeset/254691/webkit
Comment on attachment 385171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385171&action=review > Tools/Scripts/webkitpy/port/base.py:904 > + 'JSC_useKernTCSM', It's not immediately clear why this doesn't include an __XPC_DYLD variant. > Tools/Scripts/webkitpy/port/driver.py:440 > + environment['__XPC_JSC_useKernTCSM'] = environment['JSC_useKernTCSM'] This looks incorrect, should it be __XPC_DYLD_JSC_useKernTCSM
Comment on attachment 385171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385171&action=review >> Tools/Scripts/webkitpy/port/driver.py:440 >> + environment['__XPC_JSC_useKernTCSM'] = environment['JSC_useKernTCSM'] > > This looks incorrect, should it be __XPC_DYLD_JSC_useKernTCSM __XPC_ is correct. Should not be adding DYLD.
Yes, you are correct of course.
Created attachment 396455 [details] Patch
(In reply to Jonathan Bedard from comment #21) > Created attachment 396455 [details] > Patch Tested this again with layout tests, verified that it's about a 10% speed up (ran only fast). I think we should land this.
Comment on attachment 396455 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396455&action=review Seems like there is agreement about that, marking r+. > Tools/Scripts/run-javascriptcore-tests:526 > +if (isAppleMacWebKit() || isEmbeddedWebKit()) { Does this work for embedded, and is it necessary? > Tools/Scripts/webkitpy/port/driver.py:444 > + > + Two newlines?
Comment on attachment 396455 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396455&action=review >> Tools/Scripts/run-javascriptcore-tests:526 >> +if (isAppleMacWebKit() || isEmbeddedWebKit()) { > > Does this work for embedded, and is it necessary? Looking through the code, seems like it will not work for embedded builds (it only works on x86, to be more specific)
Created attachment 396532 [details] Patch
Committed r260130: <https://trac.webkit.org/changeset/260130> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396532 [details].
Remove expectations for this test in https://trac.webkit.org/changeset/260141/webkit