Bug 206436

Summary: Air O0 should have better stack allocation
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: 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 Flags
patch
none
patch
none
patch
tzagallo: review+
patch for landing none

Saam Barati
Reported 2020-01-17 13:01:12 PST
We already have liveness, so using it for a simple stack allocator should be easy
Attachments
patch (11.85 KB, patch)
2020-01-17 13:42 PST, Saam Barati 		no flags
DetailsFormatted DiffDiff
patch (11.83 KB, patch)
2020-01-17 14:31 PST, Saam Barati 		no flags
DetailsFormatted DiffDiff
patch (11.85 KB, patch)
2020-01-17 14:33 PST, Saam Barati 		tzagallo: review+
DetailsFormatted DiffDiff
patch for landing (11.98 KB, patch)
2020-01-17 17:30 PST, Saam Barati 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Saam Barati
Comment 1 2020-01-17 13:01:46 PST
This fixes our Wasm stack overflow issues
Saam Barati
Comment 2 2020-01-17 13:01:59 PST
*** Bug 201026 has been marked as a duplicate of this bug. ***
Saam Barati
Comment 3 2020-01-17 13:02:08 PST
*** Bug 200918 has been marked as a duplicate of this bug. ***
Saam Barati
Comment 4 2020-01-17 13:42:57 PST
Created attachment 388084 [details] patch I just need to adjust my stack limit test since we change stack size in our tests
Saam Barati
Comment 5 2020-01-17 14:31:57 PST
Created attachment 388090 [details] patch
Saam Barati
Comment 6 2020-01-17 14:33:45 PST
Created attachment 388091 [details] patch
Saam Barati
Comment 7 2020-01-17 14:35:06 PST
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"
Saam Barati
Comment 8 2020-01-17 14:48:29 PST
*** Bug 206419 has been marked as a duplicate of this bug. ***
Tadeu Zagallo
Comment 9 2020-01-17 15:06:10 PST
Comment on attachment 388091 [details] patch r=me
Radar WebKit Bug Importer
Comment 10 2020-01-17 17:29:52 PST
<rdar://problem/58702105>
Saam Barati
Comment 11 2020-01-17 17:30:41 PST
Created attachment 388122 [details] patch for landing
WebKit Commit Bot
Comment 12 2020-01-17 19:24:02 PST
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.
WebKit Commit Bot
Comment 13 2020-01-17 19:24:35 PST
Comment on attachment 388122 [details] patch for landing Clearing flags on attachment: 388122 Committed r254788: <https://trac.webkit.org/changeset/254788>
WebKit Commit Bot
Comment 14 2020-01-17 19:24:37 PST
All reviewed patches have been landed. Closing bug.
Aakash Jain
Comment 15 2020-01-20 09:12:55 PST
> 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
Saam Barati
Comment 16 2020-01-20 10:37:21 PST
(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?
Saam Barati
Comment 17 2020-01-20 10:41:31 PST
(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?
Saam Barati
Comment 18 2020-01-20 10:43:43 PST
(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.
Aakash Jain
Comment 19 2020-01-20 10:47:08 PST
(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
Aakash Jain
Comment 20 2020-01-20 10:48:17 PST
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.
Alexey Proskuryakov
Comment 21 2020-01-20 12:52:17 PST
The regression is tracked in bug 206477.
Joey Krug
Comment 22 2020-01-29 18:55:25 PST
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
Note You need to log in before you can comment on or make changes to this bug.