| Summary: | Air O0 should have better stack allocation | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | aakash_jain, ap, benjamin, commit-queue, fpizlo, ggaren, gskachkov, guijemont, joeykrug, keith_miller, kepounce, mark.lam, msaboff, philipp.gloor, rmorisset, ticaiolima, tzagallo, webkit-bot-watchers-bugzilla, webkit-bug-importer, ysuzuki | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=206477 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Saam Barati
2020-01-17 13:01:12 PST
This fixes our Wasm stack overflow issues *** Bug 201026 has been marked as a duplicate of this bug. *** *** Bug 200918 has been marked as a duplicate of this bug. *** Created attachment 388084 [details]
patch
I just need to adjust my stack limit test since we change stack size in our tests
Created attachment 388090 [details]
patch
Created attachment 388091 [details]
patch
Comment on attachment 388091 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=388091&action=review > Source/JavaScriptCore/ChangeLog:9 > + code creating huge stack frames and causing simple Wasm code to stack I'll fix "and causing" => "was causing" *** Bug 206419 has been marked as a duplicate of this bug. *** Comment on attachment 388091 [details]
patch
r=me
Created attachment 388122 [details]
patch for landing
The commit-queue encountered the following flaky tests while processing attachment 388122 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch. Comment on attachment 388122 [details] patch for landing Clearing flags on attachment: 388122 Committed r254788: <https://trac.webkit.org/changeset/254788> All reviewed patches have been landed. Closing bug. > Committed r254788: <https://trac.webkit.org/changeset/254788>
This seems to have broken 6 JSC tests, as also indicated by EWS.
JSC stress test failures:
- mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla
- mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-baseline
- mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-no-ftl
- mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-llint
- mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-dfg-eager-no-cjit-validate-phases
- mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-ftl-eager-no-cjit-validate-phases
(In reply to Aakash Jain from comment #15) > > Committed r254788: <https://trac.webkit.org/changeset/254788> > This seems to have broken 6 JSC tests, as also indicated by EWS. > > JSC stress test failures: > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-baseline > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-no-ftl > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-llint > - > mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-dfg-eager-no-cjit- > validate-phases > - > mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-ftl-eager-no-cjit- > validate-phases This seems super unlikely. These aren't wasm tests, right? (In reply to Saam Barati from comment #16) > (In reply to Aakash Jain from comment #15) > > > Committed r254788: <https://trac.webkit.org/changeset/254788> > > This seems to have broken 6 JSC tests, as also indicated by EWS. > > > > JSC stress test failures: > > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla > > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-baseline > > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-no-ftl > > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-llint > > - > > mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-dfg-eager-no-cjit- > > validate-phases > > - > > mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-ftl-eager-no-cjit- > > validate-phases > > This seems super unlikely. These aren't wasm tests, right? Also, a lot of these tests can't even hit this code path. Are you sure it wasn't a patch before mine? (In reply to Saam Barati from comment #17) > (In reply to Saam Barati from comment #16) > > (In reply to Aakash Jain from comment #15) > > > > Committed r254788: <https://trac.webkit.org/changeset/254788> > > > This seems to have broken 6 JSC tests, as also indicated by EWS. > > > > > > JSC stress test failures: > > > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla > > > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-baseline > > > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-no-ftl > > > - mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-llint > > > - > > > mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-dfg-eager-no-cjit- > > > validate-phases > > > - > > > mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-ftl-eager-no-cjit- > > > validate-phases > > > > This seems super unlikely. These aren't wasm tests, right? > > Also, a lot of these tests can't even hit this code path. Are you sure it > wasn't a patch before mine? Ugh, maybe it was my use of "requireOptions". I wonder if we forgot to clear that value and then we end up running this test with a super small stack height. (In reply to Saam Barati from comment #17) > Also, a lot of these tests can't even hit this code path. Are you sure it wasn't a patch before mine? r254787 passed in https://build.webkit.org/builders/Apple-Catalina-Release-JSC-Tests/builds/145 254788 failed in https://build.webkit.org/builders/Apple-Catalina-Release-JSC-Tests/builds/146 From https://build.webkit.org/builders/Apple-Catalina-Release-JSC-Tests/builds/146/steps/jscore-test/logs/stdio mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-no-ftl: Exception: RangeError: Maximum call stack size exceeded. mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-llint: Exception: RangeError: Maximum call stack size exceeded. mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-baseline: Exception: RangeError: Maximum call stack size exceeded. mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-dfg-eager-no-cjit-validate-phases: Exception: RangeError: Maximum call stack size exceeded. mozilla-tests.yaml/js1_5/Regress/regress-191633.js.mozilla-ftl-eager-no-cjit-validate-phases: Exception: RangeError: Maximum call stack size exceeded. The regression is tracked in bug 206477. Noticed this is still happening, getting RangeError: Maximum call stack size exceeded. upon instantiation of WASM code. If I refresh, it tends to fix it, but only if I refresh things. On older safari (13.0) it doesn't work at all, so this is an improvement, but it doesn't seem to be totally fixed |