-
Notifications
You must be signed in to change notification settings - Fork 706
wasm: Add wasm support #976
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tmc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @tmc. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
|
||
| // Allow WASM-specific transaction initialization | ||
| if runtime.GOARCH == "wasm" { | ||
| if initer, ok := any(db).(txIniter); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tmc for your prompt follow up 🚀 👍🏽
| if initer, ok := any(db).(txIniter); ok { | |
| if initer, ok := db.(txIniter); ok { |
wdyt about this ?
Any reason to wrap db within any ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @tmc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type assertions must take an interface value and db is *DB which means it can't be used in a type assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
I suggest holding this pull request until we make #986 work and find a solution to cross-compile in other architectures, so we're able to catch issues in new ones early/automated, rather than waiting for someone to find out compile/build issues. |
Add support for running bbolt in WebAssembly environments (js/wasm and wasip1): - Implement memory management for WASM platforms without mmap - Add platform-specific transaction initialization - Configure smaller page size and memory limits for WASM - Add GitHub workflow for WASM testing (js and wasip1) - Adjust tests to handle WASM memory constraints - Support both browser (js) and WASI environments Signed-off-by: Travis Cline <[email protected]>
Replace os.Getpagesize() calls with common.DefaultPageSize constant and remove redundant pagesize.go implementation. Simplify WASM-specific code by removing custom initialization and page size handling. Update tests to use the new constant. Signed-off-by: Travis Cline <[email protected]>
Update the db_test.go file to accommodate the recent WASM-related changes, including simplifying platform-specific page size handling and adjusting tests to handle WASM memory constraints. Signed-off-by: Travis Cline <[email protected]>
|
Thanks @tmc for your contribution 🙏🏽 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if runtime.GOARCH == "wasm" { | ||
| if initer, ok := any(db).(txIniter); ok { | ||
| if err := initer.txInit(); err != nil { | ||
| db.metalock.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to precede this call with defer ?
ideally it should be move above, after acquiring the lock
| if runtime.GOARCH == "wasm" { | ||
| if initer, ok := any(db).(txIniter); ok { | ||
| if err := initer.txInit(); err != nil { | ||
| db.rwlock.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
Do not get time to dig into the details, but my immediately feeling is that this PR isn't well integrated into the existing implementations,
Pls do not get me wrong, I am supportive to add wasm support. |
Add WebAssembly (WASM) Support to bbolt
This PR adds comprehensive support for WebAssembly targets (js and wasip1) to bolt, enabling the database to run in browsers and WASI-compliant runtimes.
What's Changed
need manual refresh
Testing Infrastructure
Test Adaptations
Testing
All tests pass on WASM platforms:
The implementation has been tested with both wazero and wasmtime runtimes.