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.

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

Add OPFS support to Wasm bindings #594

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

elijahmorg
Copy link

Lots of cleanup still left to do. Draft PR for adding support for OPFS for WASM build (add support for limbo in browser).

Overall explanation of the architecture: this follows the sqlite wasm architecture for OPFS.

main <> (limbo-worker.js) limbo (VFS - opfs.js) <> opfs-sync-proxy

The main thread loads limbo-worker.js which bootstraps the opfs.js and opfs-sync-proxy.js and then launches the limbo-wasm.js.

At that point it can be used with worker.postmessage and worker.onmessage interactions from the main thread.

The VFS provided by opfs.js provides a sync API by offloading async operations to opfs-sync-proxy.js. This is done through SharedArrayBuffer and Atomic.wait() to make the actual async operations appear synchronous for limbo.

Happy to elaborate more.

I will start cleaning this up after I get a conceptual go ahead.

resolves #531

Add basic limbo-opfs-test.html
Remove unused browser-its structure - we may bring it back
Change how VFS gets loaded based on feature flags
@penberg penberg changed the title feat: add support for OPFS Add OPFS support to Wasm bindings Jan 2, 2025
@penberg
Copy link
Collaborator

penberg commented Jan 2, 2025

Hey @elijahmor, looks great! I am totally fine with merging this after a little bit of cleanup. I do want to later on explore if we can leverage Limbo's async core more, but I think following the battle-tested SQLite approach is good.

Btw, as follow up, how do we want to package this? Do we need two separate npm packages or is there a way to somehow unify the two?

@elijahmorg
Copy link
Author

elijahmorg commented Jan 2, 2025

Awesome. I'll start cleaning it up tomorrow!

As for async my understanding is that Limbo is using its own event loop (I have not dug into that area yet).

With async in the browser you have to somehow yield to the js event loop. That was the circle I couldn't square with my current understanding of browser js/wasm/rust.

It's something I'm happy to look deeper into, but it'll take me sometime to understand limbos async architecture better.

@penberg
Copy link
Collaborator

penberg commented Jan 3, 2025

@elijahmorg Limbo is designed for an external I/O loop via the limbo_core::IO trait that the Wasm bindings also implement. But again, I think it's fine to initially go with this approach because it's simpler.

@elijahmorg
Copy link
Author

That's very interesting! I'll take a look at that, but yes I'll keep this approach for now.

General cleanup of cruft, deleted dead code, moved html files to clean
up dir.
"devDependencies": {
"@playwright/test": "^1.40.0",
"@vitest/ui": "^1.0.0",
"happy-dom": "^12.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a critical security vulnerability on this version of happy-dom, could you upgrade it?

FEATURE="web"
fi

wasm-pack build --no-pack --target $TARGET --no-default-features --features $FEATURE
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we add wasm-pack as an npm dev dependency and call npx wasm-pack here?

@penberg
Copy link
Collaborator

penberg commented Jan 6, 2025

Hey @elijahmorg, any reason to keep this as draft? Even if incomplete, we could merge this to make it easier for others to also contribute on this.

@elijahmorg
Copy link
Author

Hey! I'll try to finish my cleanup tonight after work and get this ready to merge.

I was out of town this weekend and didn't get to put any time into this.

@elijahmorg
Copy link
Author

I was running into an intermittent test failure with vitest and playwright.

Didn't want to merge intermittent test failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Browser storage support in WebAssembly bindings
3 participants