REGRESSION (iOS 16): RELEASE_ASSERT in slow_path_wasm_loop_osr #2105

krevis-figma · 2022-07-05T23:42:59Z

`a1935ab`

REGRESSION (iOS 16): RELEASE_ASSERT in slow_path_wasm_loop_osr
https://bugs.webkit.org/show_bug.cgi?id=242294

Reviewed by Yusuke Suzuki.

Fix a regression caused by https://bugs.webkit.org/show_bug.cgi?id=234587.

This affected only iOS, and only when the WebAssembly module was greater
than 10 MB. In that case, the option `webAssemblyBBQAirModeThreshold`
caused JavaScriptCore/wasm/WasmBBQPlan.cpp to fall back from Air to B3.
However, the change in 234587 in `loop_osr` didn't correctly handle that
fallback. Changed it to use the old code path in this case.

Test: None (to be added later by @justinmichaud)

* Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:
(JSC::Wasm::BBQPlan::planGeneratesLoopOSREntrypoints): Extracted from compileFunction.
(JSC::Wasm::BBQPlan::compileFunction):
* Source/JavaScriptCore/wasm/WasmBBQPlan.h:
* Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:
(JSC::LLInt::WASM_SLOW_PATH_DECL): If planGeneratesLoopOSREntrypoints, use the old code path.

Canonical link: https://commits.webkit.org/252167@main

Constellation

r=me with comments.

Constellation · 2022-07-05T23:43:38Z

Source/JavaScriptCore/wasm/WasmBBQPlan.cpp

+    // as BBQ Air bloats such lengthy Wasm code and will consume a large amount of executable memory.
+    bool forceUsingB3 = false;
+    if (Options::webAssemblyBBQAirModeThreshold() && moduleInformation->codeSectionSize >= Options::webAssemblyBBQAirModeThreshold())
+        forceUsingB3 = true;


Let's do early return return true;

Constellation · 2022-07-05T23:43:46Z

Source/JavaScriptCore/wasm/WasmBBQPlan.cpp

+    // FIXME: Some webpages use very large Wasm module, and it exhausts all executable memory in ARM64 devices since the size of executable memory region is only limited to 128MB.
+    // The long term solution should be to introduce a Wasm interpreter. But as a short term solution, we introduce heuristics to switch back to BBQ B3 at the sacrifice of start-up time,
+    // as BBQ Air bloats such lengthy Wasm code and will consume a large amount of executable memory.
+    bool forceUsingB3 = false;


We do not need to have this variable.

Constellation · 2022-07-05T23:44:08Z

Source/JavaScriptCore/wasm/WasmBBQPlan.cpp

+    else if (!Options::wasmBBQUsesAir())
+        forceUsingB3 = true;


We can make it

if (!Options::wasmBBQUsesAir()) return true;

Constellation · 2022-07-05T23:44:24Z

Source/JavaScriptCore/wasm/WasmBBQPlan.cpp

+        forceUsingB3 = true;
+    else if (!Options::wasmBBQUsesAir())
+        forceUsingB3 = true;
+    return forceUsingB3;


Let's make it return false by converting the above if-else into early return.

Constellation · 2022-07-05T23:44:43Z

Source/JavaScriptCore/wasm/WasmBBQPlan.h

@@ -75,6 +75,8 @@ class BBQPlan final : public EntryPlan {
        return Base::parseAndValidateModule(m_source.data(), m_source.size());
    }

+    static bool forceUsingB3(Ref<ModuleInformation>);


Let's take const ModuleInformation&

Constellation · 2022-07-05T23:45:21Z

Source/JavaScriptCore/wasm/WasmBBQPlan.cpp

@@ -60,6 +60,19 @@ BBQPlan::BBQPlan(Context* context, Ref<ModuleInformation> moduleInformation, uin
    dataLogLnIf(WasmBBQPlanInternal::verbose, "Starting BBQ plan for ", functionIndex);
 }

+bool BBQPlan::forceUsingB3(Ref<ModuleInformation> moduleInformation)


Let's take const ModuleInformation&.

I would suggest naming this function planGeneratesLoopOSREntrypoints or something along those lines.

Constellation · 2022-07-05T23:45:43Z

Source/JavaScriptCore/wasm/WasmBBQPlan.cpp

-        forceUsingB3 = true;
-
-    if (forceUsingB3)
+    if (forceUsingB3(m_moduleInformation))


Let's pass it by

if (forceUsingB3(m_moduleInformation.get()))

justinmichaud · 2022-07-06T00:11:47Z

Let's remove the test changes, I can land my test case separately.

krevis-figma · 2022-07-06T00:39:23Z

Thanks, followed all of your suggestions.

Constellation · 2022-07-06T01:39:20Z

Will put merge-queue label after checking EWS tests are passed.

justinmichaud · 2022-07-06T01:40:47Z

Thanks! I think I got a bit confused, the function name is currently inverted. So maybe flip the condition or negate the name of planGeneratesLoopOSREntrypoints? Otherwise, LGTM!

krevis-figma · 2022-07-06T01:53:11Z

Flipped the condition.

@justinmichaud

https://bugs.webkit.org/show_bug.cgi?id=242294 Reviewed by Yusuke Suzuki. Fix a regression caused by https://bugs.webkit.org/show_bug.cgi?id=234587. This affected only iOS, and only when the WebAssembly module was greater than 10 MB. In that case, the option `webAssemblyBBQAirModeThreshold` caused JavaScriptCore/wasm/WasmBBQPlan.cpp to fall back from Air to B3. However, the change in 234587 in `loop_osr` didn't correctly handle that fallback. Changed it to use the old code path in this case. Test: None (to be added later by @justinmichaud) * Source/JavaScriptCore/wasm/WasmBBQPlan.cpp: (JSC::Wasm::BBQPlan::planGeneratesLoopOSREntrypoints): Extracted from compileFunction. (JSC::Wasm::BBQPlan::compileFunction): * Source/JavaScriptCore/wasm/WasmBBQPlan.h: * Source/JavaScriptCore/wasm/WasmSlowPaths.cpp: (JSC::LLInt::WASM_SLOW_PATH_DECL): If planGeneratesLoopOSREntrypoints, use the old code path. Canonical link: https://commits.webkit.org/252167@main

webkit-commit-queue · 2022-07-06T03:10:57Z

Committed 252167@main (a1935ab): https://commits.webkit.org/252167@main

Reviewed commits have been landed. Closing PR #2105 and removing active labels.

krevis-figma requested a review from a team as a code owner July 5, 2022 23:42

Constellation approved these changes Jul 5, 2022

View reviewed changes

krevis-figma force-pushed the eng/242294 branch from e182d4d to ea13519 Compare July 6, 2022 00:30

krevis-figma force-pushed the eng/242294 branch from ea13519 to f688bc8 Compare July 6, 2022 01:51

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jul 6, 2022

Constellation added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Jul 6, 2022

webkit-early-warning-system force-pushed the eng/242294 branch from f688bc8 to a1935ab Compare July 6, 2022 03:10

webkit-early-warning-system merged commit a1935ab into WebKit:main Jul 6, 2022

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REGRESSION (iOS 16): RELEASE_ASSERT in slow_path_wasm_loop_osr #2105

REGRESSION (iOS 16): RELEASE_ASSERT in slow_path_wasm_loop_osr #2105

krevis-figma commented Jul 5, 2022 •

edited by webkit-commit-queue

Constellation left a comment

Constellation Jul 5, 2022

Constellation Jul 5, 2022

Constellation Jul 5, 2022

Constellation Jul 5, 2022

Constellation Jul 5, 2022

Constellation Jul 5, 2022

justinmichaud Jul 6, 2022 •

edited

Constellation Jul 5, 2022

justinmichaud commented Jul 6, 2022

krevis-figma commented Jul 6, 2022

Constellation commented Jul 6, 2022

justinmichaud commented Jul 6, 2022

krevis-figma commented Jul 6, 2022

webkit-commit-queue commented Jul 6, 2022

REGRESSION (iOS 16): RELEASE_ASSERT in slow_path_wasm_loop_osr #2105

REGRESSION (iOS 16): RELEASE_ASSERT in slow_path_wasm_loop_osr #2105

Conversation

krevis-figma commented Jul 5, 2022 • edited by webkit-commit-queue

Constellation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmichaud Jul 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmichaud commented Jul 6, 2022

krevis-figma commented Jul 6, 2022

Constellation commented Jul 6, 2022

justinmichaud commented Jul 6, 2022

krevis-figma commented Jul 6, 2022

webkit-commit-queue commented Jul 6, 2022

krevis-figma commented Jul 5, 2022 •

edited by webkit-commit-queue

justinmichaud Jul 6, 2022 •

edited