Optimize BBQ for runs of jump targets in `br.table` by danlliu · Pull Request #31786 · WebKit/WebKit

danlliu · 2024-08-06T16:14:09Z

`d39f470`

Optimize BBQ for runs of jump targets in `br.table`
https://bugs.webkit.org/show_bug.cgi?id=277686
rdar://133300128

Reviewed by NOBODY (OOPS!).

Previously, BBQ generated an entire binary search for every possible case in a `br.table`,
even if we had long consecutive runs that could avoid many comparisons. This patch
optimizes `br.table` generation to reduce the tree depth when it is possible.

* Source/JavaScriptCore/jit/BinarySwitch.cpp:
(JSC::BinarySwitch::BinarySwitch):
(JSC::BinarySwitch::advance):
(JSC::BinarySwitch::buildCheckRuns):
(JSC::BinarySwitch::BranchCode::dump const):
* Source/JavaScriptCore/jit/BinarySwitch.h:
(JSC::BinarySwitch::BranchCode::BranchCode):
* Source/JavaScriptCore/wasm/WasmBBQJIT.cpp:
(JSC::Wasm::BBQJITImpl::BBQJIT::addSwitch):

d39f470

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2		✅ 🛠 jsc-armv7
	✅ 🛠 tv		✅ 🧪 jsc-armv7-tests
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-08-06T16:14:15Z

EWS run on previous version of this PR (hash 707c838)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-cairo
❌ 🛠 🧪 jsc	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
❌ 🛠 🧪 jsc-arm64	✅ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2		✅ 🛠 jsc-armv7
	✅ 🛠 tv		✅ 🧪 jsc-armv7-tests
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

kmiller68

I think we should integrate this into BinarySwitch rather than add a new class. That could be done by adding a new constructor if needed.

kmiller68 · 2024-08-06T16:23:00Z

This whitespace change seems wrong.

kmiller68 · 2024-08-06T16:34:10Z

This could just be out.print(kind);

webkit-early-warning-system · 2024-08-06T19:19:22Z

EWS run on previous version of this PR (hash 8c5b442)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	~~🧪 wincairo-tests~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	~~🧪 api-wpe~~
	✅ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	✅ 🛠 wpe-cairo
~~🛠 🧪 jsc~~	✅ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 vision	~~🧪 mac-AS-debug-wk2~~	~~🧪 gtk-wk2~~
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2		✅ 🛠 jsc-armv7
	~~🛠 tv~~		✅ 🧪 jsc-armv7-tests
	~~🛠 tv-sim~~
	~~🛠 watch~~
	~~🛠 watch-sim~~

https://bugs.webkit.org/show_bug.cgi?id=277686 rdar://133300128 Reviewed by NOBODY (OOPS!). Previously, BBQ generated an entire binary search for every possible case in a `br.table`, even if we had long consecutive runs that could avoid many comparisons. This patch optimizes `br.table` generation to reduce the tree depth when it is possible. * Source/JavaScriptCore/jit/BinarySwitch.cpp: (JSC::BinarySwitch::BinarySwitch): (JSC::BinarySwitch::advance): (JSC::BinarySwitch::buildCheckRuns): (JSC::BinarySwitch::BranchCode::dump const): * Source/JavaScriptCore/jit/BinarySwitch.h: (JSC::BinarySwitch::BranchCode::BranchCode): * Source/JavaScriptCore/wasm/WasmBBQJIT.cpp: (JSC::Wasm::BBQJITImpl::BBQJIT::addSwitch):

webkit-early-warning-system · 2024-08-06T20:54:56Z

EWS run on current version of this PR (hash d39f470)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2		✅ 🛠 jsc-armv7
	✅ 🛠 tv		✅ 🧪 jsc-armv7-tests
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

hyjorc1 · 2024-08-06T21:11:59Z

+        while (wasmSwitch.advance(m_jit)) {
+            unsigned value = wasmSwitch.caseValue();
+            unsigned index = wasmSwitch.caseIndex();
+            UNUSED_VARIABLE(value);


Just remove value?

Yeah, just get rid of value

kmiller68 · 2024-08-06T22:30:34Z

-        m_cases.append(Case(cases[i], i));
-
-    std::sort(m_cases.begin(), m_cases.end());
+    if (type == Int32 || type == IntPtr) {


Nit: I would add a isCheckRuns() and use that.

kmiller68 · 2024-08-06T22:32:21Z

+        while (wasmSwitch.advance(m_jit)) {
+            unsigned value = wasmSwitch.caseValue();
+            unsigned index = wasmSwitch.caseIndex();
+            UNUSED_VARIABLE(value);


Yeah, just get rid of value

kmiller68 · 2024-08-06T22:33:14Z

 #endif
+        build(0, false, m_cases.size());
+    } else {
+        m_cases.reserveInitialCapacity(cases.size());


cases are expected to be sorted right? Can we add an ASSERT?

chicoxyzzy · 2026-07-15T16:16:00Z

Sorry for the noise, I didn’t notice this PR when I picked up the bug. It was unassigned on Bugzilla and I only saw the open work after I had already opened a new patch.

I posted a separate attempt here: #69487

At a high level it tries the same optimization (collapse runs of the same br.table target into a shallower BinarySwitch), with a few differences:

only consecutive same targets (no remapping of non-adjacent ones)
unsigned range checks for the run tree
jump table vs binary based on run count rather than raw table size
a small stress test

Happy to close #69487 if you’d rather continue this one, help review / contribute to this PR, or fold any of those bits in here if useful.

danlliu requested a review from a team as a code owner August 6, 2024 16:14

danlliu self-assigned this Aug 6, 2024

danlliu added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Aug 6, 2024

kmiller68 requested changes Aug 6, 2024

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 6, 2024

danlliu removed the merging-blocked Applied to prevent a change from being merged label Aug 6, 2024

danlliu force-pushed the eng/Optimize-BBQ-for-runs-of-jump-targets-in-br-table branch from 707c838 to 8c5b442 Compare August 6, 2024 18:51

danlliu force-pushed the eng/Optimize-BBQ-for-runs-of-jump-targets-in-br-table branch from 8c5b442 to d39f470 Compare August 6, 2024 20:54

hyjorc1 reviewed Aug 6, 2024

View reviewed changes

kmiller68 reviewed Aug 6, 2024

View reviewed changes

kmiller68 self-requested a review August 6, 2024 22:36

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 6, 2024

Navigation Menu

Uh oh!

Optimize BBQ for runs of jump targets in `br.table` - #31786

Optimize BBQ for runs of jump targets in `br.table`#31786
danlliu wants to merge 1 commit into
WebKit:mainfrom
danlliu:eng/Optimize-BBQ-for-runs-of-jump-targets-in-br-table

danlliu commented Aug 6, 2024 •

edited by webkit-early-warning-system

Loading

Uh oh!

webkit-early-warning-system commented Aug 6, 2024 •

edited

Loading

Uh oh!

kmiller68 left a comment

Uh oh!

kmiller68 Aug 6, 2024

Uh oh!

kmiller68 Aug 6, 2024

Uh oh!

webkit-early-warning-system commented Aug 6, 2024 •

edited

Loading

Uh oh!

webkit-early-warning-system commented Aug 6, 2024 •

edited

Loading

Uh oh!

hyjorc1 Aug 6, 2024

Uh oh!

kmiller68 Aug 6, 2024

Uh oh!

kmiller68 Aug 6, 2024

Uh oh!

kmiller68 Aug 6, 2024

Uh oh!

kmiller68 Aug 6, 2024

Uh oh!

chicoxyzzy commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

danlliu commented Aug 6, 2024 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmiller68 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chicoxyzzy commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danlliu commented Aug 6, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Aug 6, 2024 •

edited

Loading

webkit-early-warning-system commented Aug 6, 2024 •

edited

Loading

webkit-early-warning-system commented Aug 6, 2024 •

edited

Loading