- 
 
- 
  Notifications
 You must be signed in to change notification settings 
- Fork 57
BridgeJS: Support for multiple associated values in enums using binary buffer format #436
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
BridgeJS: Support for multiple associated values in enums using binary buffer format #436
Conversation
d3a5164 to
 516e46b  
 Compare
 
 | Sorry for taking a long time 🙇 Option 1: Memory-encoding ABI (current in this PR)Encode each parameter into a byte buffer on the JS side, copy into Wasm memory, and decode on the Swift side. I think it's very straightforward but I see some drawbacks: 
 I think we have some perf optimization space here but my major concern is its implementation complexity Option 2: Stack-based ABIPush Wasm core values of lowered parameter representation onto a temporary JS stack, pop them from Swift and lift them by reusing  Similar to what you've done for result lifting here. We can apply it to both parameter passing and result returning. lower: (value) => { const enumTag = value.tag; switch (enumTag) { case APIResult.Tag.Success: { const bytes = textEncoder.encode(value.param0); const bytesId = swift.memory.retain(bytes); tmpParamI32s.push(bytes.byteLength); tmpParamI32s.push(bytesId); return; } case APIResult.Tag.Failure: { tmpParamI32s.push(value.param0); return; } case APIResult.Tag.Flag: { tmpParamI32s.push(value.param0 ? 1 : 0); return; } case APIResult.Tag.Rate: { tmpParamF64s.push(value.param0); return; } case APIResult.Tag.Precise: { tmpParamF64s.push(value.param0); return; } case APIResult.Tag.Info: { return; } default: throw new Error("Unknown APIResult tag: " + String(enumTag)); } }, addImports: (importObject, importsContext) => { ... bjs["swift_js_pop_param_int32"] = function() { return tmpParamI32s.pop(); } bjs["swift_js_pop_param_float64"] = function() { return tmpParamF64s.pop(); } } private extension APIResult { static func bridgeJSLiftParameter(_ caseId: Int32) -> APIResult { switch caseId { case 0: return .success(String.bridgeJSLiftParameter(_swift_js_pop_param_int32(), _swift_js_pop_param_int32())) case 1: return .failure(Int.bridgeJSLiftParameter(_swift_js_pop_param_int32())) case 2: return .flag(Bool.bridgeJSLiftParameter(_swift_js_pop_param_int32())) case 3: return .rate(Float.bridgeJSLiftParameter(_swift_js_pop_param_float64())) case 4: return .precise(Double.bridgeJSLiftParameter(_swift_js_pop_param_float64())) case 5: return .info default: fatalError("Unknown APIResult case ID: \(caseId)") } } } Pros 
 Cons 
 Option 3: Flattened ABIUse a fixed slot layout (e.g.,  function lower(value) { switch (value.tag) { case APIResult.Tag.Flag: return [1, 0, 0]; // (i1=1, i2=0, f=0) case APIResult.Tag.Precise: return [0, 0, value.param0]; // (i1=0, i2=0, f=f64) // ... } } instance.exports.bjs_EnumRoundtrip_take(selfPtr, value.tag, i1, i2, f); @_cdecl("bjs_EnumRoundtrip_take") func bjs_EnumRoundtrip_take(_ selfPtr: UnsafeMutableRawPointer, _ tag: Int32, _ i1: Int32, _ i2: Int32, _ f: Double) { switch tag { case 1: take(.failure(Int(i1))) case 2: take(.flag(i1 != 0)) case 3: take(.rate(Float(f))) case 4: take(.precise(f)) case 5: take(.info) } } Pros 
 Cons 
 BenchmarkI conducted a benchmark with a hacky manual patches. (you can see benchmark code on my branch. I manually modified the generated bridge-js.js other than the change tracked by git flat-abi.js stack-abi.js) 
 My proposalGiven this, Option 2 looks like a reasonable baseline candidate, and Option 3 could be layered on as an optimization for fixed storage size types in the future. Does this direction make sense to you, or do you see other trade-offs we should weigh? | 
@kateinoigakukun thanks for in-depth analysis 🙇🏼♂️
Given your benchmark results, I think we should go with option 2, it seems like an overall approach we could reuse later when adding new types and that one seems more scalable for constructs with multiple values like case comprehensive(Bool, Bool, Int, Int, Double, Double, String, String, String) comparing to option 3.
I'll start migration to option 2 and update PR when ready 🙏🏻
568df63 to
 1e1952d  
 Compare
 
 @kateinoigakukun PR updated for stack based solutions, couple of notes:
- 2 set of stacks for ret / par to avoid conflicts on roundtrips
- no particular place to clear the stacks, as we generate equal numbers of push / pop, no need for that
- normal order on Swift side, inverse order on JS side
- kept swift_js_push_stringseparate fromswift_js_return_string(comparing to initial PR) to not mix those up
- bridgeJSLiftParameterreuses existing- bridgeJSLiftParametermechanisms, while- bridgeJSLowerReturnneeds to use- _swift_js_push<type>as default- bridgeJSLowerReturnmethods would not use stacks
- it's fine to pass string to bridgeJSLiftParameteras 2 int32, but we needwithUTF8for lowering, hence strings stack
I will add comprehensive benchmark on separate branch to confirm performance based on parts of your branch, so will either add this to this PR or issue another PR against this branch 🙏🏻
1e1952d to
 e89dfc0  
 Compare
 
 | @kateinoigakukun here is the comparison for before / after changes 👌🏻 EnumRoundtrip Comparison
 ComplexResultRoundtrip Comparison
 Here is the branch with working benchmarks for both enums and previous approach: feat/bridgejs-benchmark-buffer | 
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.
Looks good to me. I think we can centralize JS glue codegen for lifting/lowering payload types by reusing IntrinsicJSFragment but, let's incubate on the main branch!
Thanks as always!
f9a09a3
 into
 
 
 swiftwasm:main
 
 @kateinoigakukun I'll give it a go after the weekend to use IntrinsicJSFragment for helper functions fragments and align approach, so will issue a new PR with that, thanks for suggestion! 👍
Introduction
This PR support for enums with associated values of primitive types to the BridgeJS plugin.
Design Overview
I've decided to go with DataView buffer to encode short type information and then payload, to pass single value across the boundaries and unpack it on the other side.
I started with single value primitives using what is currently available, then migrated to serializing parameters into JSON string and with current solution
Other option I've considered but rejected was to generate a separate WASM export function for each enum case - this eliminates the need for case dispatching and parameter packing, but on the other hand it would lead to WASM export explosion given that each case in each enum would end up as separate function and multiple associated values within single case would lead to unreadable long parameters chains.
Alternatives
As discussed, other alternative would be to allocate value types on Swift side, box it and use handle + accessors for access on JS side similarly to classes.
But as for this, we should probably decide on high level how we want to treat value types and whether we want to focus on just one of those options or use them depending on type, amount of properties or estimated payload size.
Examples
Case Enum with Both Styles
Generated TypeScript:
Testing
Added tests with different associated values, testing multiple properties of same type (as this is new addition) and working functions for nested enums based on last fix
Docs
Extended enum section in newly adjusted documentation