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

Generalize WASI to other Wasm engines #79

Merged
merged 8 commits into from
Apr 17, 2024

Conversation

kabiroberai
Copy link
Contributor

@kabiroberai kabiroberai commented Apr 17, 2024

This PR enables layering the WASI module on top of other Wasm engines, such as JavaScriptCore.

To achieve this, it's worth first noting that the only real dependency that WASI had on WasmKit was in reading/writing host memory (via UnsafeGuestPointer) as well as the small set of core Wasm Value types. Accordingly, I've split these bits out into a separate WasmTypes module and created a WASIBase package that depends only on WasmTypes. I created a BaseGuestMemory protocol that can be implemented by engines, and kept the GuestMemory type as WasmKit's concrete implementation.

I believe this maintains full source compatibility with previous versions, but I'm not sure if the API design is ideal. Certainly open to feedback on how this could be improved

edit: there is some minor API breakage in that UnsafeGuest[Raw]Pointer now uses the protocol-ized BaseGuestMemory and requires a count when obtaining a pointer to host memory. If we really wanted full source compat we could create more pointer variants that take the concrete WasmKit.GuestMemory but I’m not sure it’s worth it.

@kabiroberai
Copy link
Contributor Author

evidently I hadn't run the tests myself yet 😶 @kateinoigakukun tests should now be green

@@ -47,3 +49,23 @@ public struct CanonicalCallContext {
return UnsafeGuestRawPointer(memorySpace: guestMemory, offset: new)
}
}

public struct GuestMemory: BaseGuestMemory {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public struct GuestMemory: BaseGuestMemory {
public struct WasmKitGuestMemory: GuestMemory {

I personally prefer avoiding Base here

Package.swift Outdated
@@ -15,6 +15,10 @@ let package = Package(
name: "WASI",
targets: ["WASI"]
),
.library(
name: "WASIBase",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name: "WASIBase",
name: "WASI",

I'd like to make this engine agnostic target as bare WASI, and the engine dependent one as WasmKitWASI

@kabiroberai
Copy link
Contributor Author

this is ready from my end! The only thing I haven't been able to test yet is the performance of using an existential any GuestMemory inside UnsafeGuestPointer as I don't see a perf suite/baseline measurements, but my gut feeling is that it won't have a material effect since it's probably overshadowed by the existing overhead of guest-to-host calls (also an extra memory access shouldn't hurt much since UnsafeGuestPointer implies there's already memory ops involved).

@kateinoigakukun
Copy link
Member

I'll merge this once I check on my local 👍

let (name, type) = entry
functions[name] = HostFunction(type: type) { _, _ in
functions[name] = WASIHostFunction(type: type) { _, _ in
print("\"\(name)\" not implemented yet")
return [.i32(WASIAbi.Errno.ENOSYS.rawValue)]
}
}

func withMemoryBuffer<T>(
Copy link
Contributor Author

@kabiroberai kabiroberai Apr 17, 2024

Choose a reason for hiding this comment

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

I should note that this function is basically now a no-op since this work is offloaded to WasmKitWASI. I wanted to minimize the diff to begin with for ease of review but it might make sense to remove this now. @kateinoigakukun would you prefer that I do it in this PR or in a follow-up?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for minimizing diff! I prefer a following up way

@kateinoigakukun kateinoigakukun merged commit d4e83dc into swiftwasm:main Apr 17, 2024
6 checks passed
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.

2 participants