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

Avoid manual memory management with JSClosure #106

Closed
MaxDesiatov opened this issue Nov 4, 2020 · 2 comments · Fixed by #128
Closed

Avoid manual memory management with JSClosure #106

MaxDesiatov opened this issue Nov 4, 2020 · 2 comments · Fixed by #128
Assignees
Labels
enhancement New feature or request

Comments

@MaxDesiatov
Copy link
Contributor

I wonder if reference types could lift our requirement to manage references to JS closures manually?

With externrefs, the host gives Wasm limited access to host objects, but the host also needs to know when Wasm is finished with those objects, so it can clean them up. That clean up might involve closing a file handle or simply deallocating memory.

This means that JS closures should be passed to the Swift code as externref. Deallocating all references to externref would deallocate the closure on the JS side.

Reference types are now available in all browsers except Safari, and in the latter case they seem to only be hidden behind JSC_useWebAssemblyReferences flag. The only question is externref support in our toolchain...

Doesn't look like LLVM had much progress in that direction? 🤔 Also see WebAssembly/tool-conventions#122

@MaxDesiatov MaxDesiatov added the enhancement New feature or request label Nov 4, 2020
@MaxDesiatov
Copy link
Contributor Author

Turns out LLVM does have it in some state as introduced in llvm/llvm-project@79aad89.

@j-f1
Copy link
Member

j-f1 commented Nov 5, 2020

IIRC the issue with JSClosure is that when passed to JS APIs, the Swift closure is deallocated early. We could manage this by using a FinalizationRegistry to track when it’s safe to free the Swift closure, but that has not yet reached production versions of Safari (it seems to currently be in nightly WebKit only)

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

Successfully merging a pull request may close this issue.

2 participants