feat(core): Strongly typed deserialization of JSON ops #9423
Conversation
|
|
|
On my local machine, http_bench_json_ops showed a 15% increase in throughput when deserializing directly to a struct. The raw numbers from wrk can be seen in this gist. The CI benchmarks show similar numbers, with a 8% increase between the fastest recent CI bench I could find on master (1834611552) and the one CI bench for this PR that got to http_bench_json_ops before getting cancelled (542109582). Finally, I calculated a 30%(!?) improvement in latency compared to the average of the last 6 commits to master, but I think the difference being that large has more to do with the large variance between runs as opposed to the code actually being that much faster. @lucacasonato, sorry I didn't notice your earlier PR. Would it make sense to include your fixes for error handling in this PR? |
|
Thanks for the PR @jbellerb; I'm a little hesitant as it is a major change, but if benchmarks are favorable we should look into that. Please make sure all tests are passing. |
If you can get this PR to work without, lets not. Let's try to keep this PR as slim as possible. We can do that in a follow up. |
|
I am very much in favor of this change. The change itself is relatively minor (only 70 LOC changed), but is very very useful for the usability of ops. I think it would allow us to get rid of a few hundred lines across all of the ops (all of the |
|
I'm in favour of this change too... It is similiar to the way the lspower works on calls, and the way we want to move the integration of tsc response values as well. |
|
@bartlomieju I am trying to change as little as possible to make upgrading easy. Unless some code depends on the internal behavior of jsonOpAsync, the only changes have to do with the function types. In that case, simple migration (without taking advantage of the new feature) should work like this: |
@jbellerb you can use already existing benchmarks to compare against |
There was a problem hiding this comment.
LGTM now. Thanks for the effort @jbellerb.
Requesting additional review from @bartlomieju, and @ry.
| dispatch(opName, argsBuf, ...zeroCopy); | ||
| const promiseId = nextPromiseId++; | ||
| const reqBuf = core.encode("\0".repeat(8) + JSON.stringify(args)); | ||
| new DataView(reqBuf.buffer).setBigUint64(0, BigInt(promiseId)); |
There was a problem hiding this comment.
So the first 8 bytes represent the promiseId encoded as a big endian u64 ? Is that only the case for JSON ops or also for minimal ops? Maybe a comment is warranted here
There was a problem hiding this comment.
Yes, it's only for JSON ops. However, it looks like the two implementation are becoming more similar (#9457). Would it make sense to instead store the promiseId in the first 4 bytes as a big endian i32, like it is for minimal ops?
There was a problem hiding this comment.
Yes that might allow them to share more code on the backend... I think it could be addressed in a follow up commit tho.
|
Thanks so much for reviewing this! |
(from #9422)
This PR makes json_op_sync/async generic to all Deserialize/Serialize types instead of the loosely-typed serde_json::Value. Since serde_json::Value implements Deserialize/Serialize, very little existing code needs to be updated, however as json_op_sync/async are now generic, type inference is broken in some cases (see cli/build.rs:146). I've found this reduces a good bit of boilerplate, as seen in the updated deno_core examples.
This change may also reduce serialization and deserialization overhead as serde has a better idea of what types it is working with. I am currently working on benchmarks to confirm this and I will update this PR with my findings.