Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JSC] Add wasm fixed-sized FuncRefTable #9092

Merged
merged 1 commit into from
Jan 25, 2023
Merged

[JSC] Add wasm fixed-sized FuncRefTable #9092

merged 1 commit into from
Jan 25, 2023

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented Jan 25, 2023 •
edited by webkit-early-warning-system

0ebf892

[JSC] Add wasm fixed-sized FuncRefTable
https://bugs.webkit.org/show_bug.cgi?id=251136
rdar://104634169

Reviewed by Mark Lam.

In many cases, FuncRefTable is fixed-sized (initial size matches against maximum size).
This patch adds FixedSized optimization for FuncRefTable:

1. We allocate function's buffer as a tail of FuncRefTable memory.
2. In BBQ and OMG code, we avoid one-level indirection of load if we know that table is fixed-sized and
   it is not imported table.

* Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h:
(JSC::Wasm::ExpressionType>::addCallIndirect):
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::addCallIndirect):
* Source/JavaScriptCore/wasm/WasmTable.cpp:
(JSC::Wasm::Table::Table):
(JSC::Wasm::Table::tryCreate):
(JSC::Wasm::FuncRefTable::FuncRefTable):
(JSC::Wasm::FuncRefTable::~FuncRefTable):
(JSC::Wasm::FuncRefTable::createFixedSized):
* Source/JavaScriptCore/wasm/WasmTable.h:
(JSC::Wasm::Table::isFixedSized const):

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

a47ec34

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 webkitpy ✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
  🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🧪 services ✅ 🛠 watch ✅ 🧪 mac-wk2-stress ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests
❌ 🛠 🧪 unsafe-merge

@Constellation Constellation requested a review from a team as a code owner January 25, 2023 06:07
@Constellation Constellation self-assigned this Jan 25, 2023
@Constellation Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Jan 25, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 25, 2023
MenloDorian
MenloDorian approved these changes Jan 25, 2023
View reviewed changes
Copy link

@MenloDorian MenloDorian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

Source/JavaScriptCore/wasm/WasmTable.cpp
@@ -88,9 +89,14 @@ RefPtr<Table> Table::tryCreate(uint32_t initial, std::optional<uint32_t> maximum
switch (type) {
case TableElementType::Externref:
return adoptRef(new ExternRefTable(initial, maximum));
case TableElementType::Funcref:
case TableElementType::Funcref: {
if (maximum && maximum.value() == initial) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not check if (isFixedSized()) instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is static function, to create a table, so isFixedSized is not computed (it is computed after creating a table).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. I mistook this for being part of the constructor above.

Source/JavaScriptCore/wasm/WasmTable.h Outdated
Comment on lines 142 to 143
static ptrdiff_t offsetOfTail() { return WTF::roundUpToMultipleOf<sizeof(Function)>(sizeof(FuncRefTable)); }
static ptrdiff_t offsetOfFunctionsForFixedSizedTable() { return offsetOfTail(); }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constexpr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 3bbddb3)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ⏳ 🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 gtk-wk2
✅ 🧪 webkitpy   🧪 api-ios ✅ 🧪 mac-wk1   🧪 api-gtk
⏳ 🛠 🧪 jsc ✅ 🛠 tv   🧪 mac-wk2 ❌ 🛠 jsc-armv7
⏳ 🛠 🧪 jsc-arm64 ✅ 🛠 tv-sim   🧪 mac-AS-debug-wk2 ❌ 🧪 jsc-armv7-tests
✅ 🧪 services ✅ 🛠 watch ✅ 🧪 mac-wk2-stress ✅ 🛠 jsc-mips
✅ 🛠 watch-sim   🧪 jsc-mips-tests

@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Jan 25, 2023
@Constellation Constellation force-pushed the eng/JSC-Add-wasm-fixed-sized-FuncRefTable branch from 3bbddb3 to a47ec34 Compare January 25, 2023 07:28
@webkit-early-warning-system
Copy link
Collaborator

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

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 webkitpy ✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
  🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🧪 services ✅ 🛠 watch ✅ 🧪 mac-wk2-stress ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests
❌ 🛠 🧪 unsafe-merge

@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 25, 2023
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/JSC-Add-wasm-fixed-sized-FuncRefTable branch from a47ec34 to 7c17226 Compare January 25, 2023 17:05
@Constellation
[JSC] Add wasm fixed-sized FuncRefTable
0ebf892 
https://bugs.webkit.org/show_bug.cgi?id=251136
rdar://104634169

Reviewed by Mark Lam.

In many cases, FuncRefTable is fixed-sized (initial size matches against maximum size).
This patch adds FixedSized optimization for FuncRefTable:

1. We allocate function's buffer as a tail of FuncRefTable memory.
2. In BBQ and OMG code, we avoid one-level indirection of load if we know that table is fixed-sized and
   it is not imported table.

* Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h:
(JSC::Wasm::ExpressionType>::addCallIndirect):
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::addCallIndirect):
* Source/JavaScriptCore/wasm/WasmTable.cpp:
(JSC::Wasm::Table::Table):
(JSC::Wasm::Table::tryCreate):
(JSC::Wasm::FuncRefTable::FuncRefTable):
(JSC::Wasm::FuncRefTable::~FuncRefTable):
(JSC::Wasm::FuncRefTable::createFixedSized):
* Source/JavaScriptCore/wasm/WasmTable.h:
(JSC::Wasm::Table::isFixedSized const):

Canonical link: https://commits.webkit.org/259363@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/JSC-Add-wasm-fixed-sized-FuncRefTable branch from 7c17226 to 0ebf892 Compare January 25, 2023 17:09
@webkit-commit-queue
Copy link
Collaborator

Committed 259363@main (0ebf892): https://commits.webkit.org/259363@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit 0ebf892 into WebKit:main Jan 25, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 25, 2023
@Constellation Constellation deleted the eng/JSC-Add-wasm-fixed-sized-FuncRefTable branch January 25, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MenloDorian MenloDorian MenloDorian approved these changes

Assignees

@Constellation Constellation

Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Milestone
No milestone
5 participants
@Constellation @webkit-early-warning-system @webkit-commit-queue @MenloDorian @webkit-ews-buildbot