prisma-engines icon indicating copy to clipboard operation
prisma-engines copied to clipboard

Integration: TS from rust

Open jkomyno opened this issue 2 years ago β€’ 6 comments

jkomyno avatar Apr 02 '23 14:04 jkomyno

CodSpeed Performance Report

Merging #3835 integration/ts-from-rust (eff7140) will improve performances by 75.32%.

Summary

πŸ”₯ 3 improvements ❌ 0 regressions βœ… 3 untouched benchmarks

πŸ†• 0 new benchmarks ⁉️ 0 dropped benchmarks

Benchmarks breakdown

Benchmark main integration/ts-from-rust Change
πŸ”₯ build (large) 7.4 s 4.2 s +75.32%
πŸ”₯ build (medium) 347.6 ms 268.4 ms +29.5%
πŸ”₯ build (small) 42.3 ms 34.6 ms +22.14%

codspeed-hq[bot] avatar Apr 02 '23 15:04 codspeed-hq[bot]

Also sharing for early feedback and to start a discussion: I still have doubts about tsify. Is IntoWasmAbi meant to be implemented like that? I'm not familiar enough with wasm-bindgen internals to say, I'd need to get more familiar with the implementation first to do a good review. We'd be taking on something more complex (always counting the dependencies), we have to be sure the gains on the typescript side are justifying the tradeoffs.

One last factor that is making me hesitant is that the wasm ↔ host boundary is not well tested or measured in other ways (e.g. basic benchmarks), so we're steering in the dark. Not an argument for status quo either, just a concern.

tomhoule avatar Apr 03 '23 06:04 tomhoule

I also had another look serde_wasm_bindgen and it makes sense now, though. It's still very green but it makes sense. That might be a way around adding wasm-bindgen dependencies to crates other than prisma-fmt-wasm.

tomhoule avatar Apr 03 '23 06:04 tomhoule

Also sharing for early feedback and to start a discussion: I still have doubts about tsify. Is IntoWasmAbi meant to be implemented like that? I'm not familiar enough with wasm-bindgen internals to say, I'd need to get more familiar with the implementation first to do a good review. We'd be taking on something more complex (always counting the dependencies), we have to be sure the gains on the typescript side are justifying the tradeoffs.

One last factor that is making me hesitant is that the wasm ↔ host boundary is not well tested or measured in other ways (e.g. basic benchmarks), so we're steering in the dark. Not an argument for status quo either, just a concern.

Structurally, this PR would remove the need for two expensive operations in JS land: JSON.stringify(inputArguments) and JSON.parse(returnedValue). I know this is faster because I've done it elsewhere, and we can use benchmarks to test the difference of the old getDmmf.test.ts (using Wasm + JSON (de)serialization in Rust + JS) vs the new getDmmf.test.ts (using Wasm + JSON (de)serialization in Rust only)

jkomyno avatar Apr 03 '23 08:04 jkomyno

I also had another look serde_wasm_bindgen and it makes sense now, though. It's still very green but it makes sense. That might be a way around adding wasm-bindgen dependencies to crates other than prisma-fmt-wasm.

Note: tsifyΒ is essentially a wrapper of serde_wasm_bindgen (currently using the jsonΒ feature flag) that's also responsible of creating type definitions that usually work out of the box, with escape hatches when needed (via the #[tsify(type = "...")] struct field macro).

jkomyno avatar Apr 03 '23 08:04 jkomyno

Structurally, this PR would remove the need for two expensive operations in JS land: JSON.stringify(inputArguments) and JSON.parse(returnedValue). I know this is faster because I've done it elsewhere, and we can use benchmarks to test the difference of the old getDmmf.test.ts (using Wasm + JSON (de)serialization in Rust + JS) vs the new getDmmf.test.ts (using Wasm + JSON (de)serialization in Rust only)

For my understanding: as it is, the PR uses the serde-json variant of tsify (because the serde-wasm-bindgen variant had problems you raised on slack), β€” I understand the potential improvements in performance from using serde-wasm-bindgen, but in this case it's not clear to me, would it make any difference with what we do now?

tomhoule avatar Apr 03 '23 08:04 tomhoule