-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
refactor(core): new optimized op-layer using serde_v8 #9843
Conversation
Handle missing values (undefined/nullish) by implementing visit_unit for ParseBooleanOrStringVec
Since otherwise casting f64 to u64 will fail, which can happen when unpacking serde_json::Values like in op_close (i.e: Value::as_u64)
Since it only tests the shared queue which no longer exists
Removing variadic zeroCopy, make it an optional arg
I've given this a cursory review and the structure looks good to me. |
Since there's no longer a shared queue to overflow
To be re-enabled and revisited prior to 1.9 release
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.
LGTM 🚀
Huge thanks @AaronO! This work will be ground-breaking for the overall performance of the runtime and I'm looking forward to removing even more code in the follow up PRs.
@@ -1,411 +1,124 @@ | |||
// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. |
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.
Awesome to see this file being 120 lines
@@ -56,12 +55,17 @@ pub use crate::modules::RecursiveModuleLoad; | |||
pub use crate::normalize_path::normalize_path; | |||
pub use crate::ops::op_close; | |||
pub use crate::ops::op_resources; | |||
pub use crate::ops::serialize_op_result; |
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.
I don't think this function should be public, what's the use case?
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.
I can make that private, I had left it public since it wasn't 100% clear how we'll approach plugin ops (though it seems like they won't need this and should be able to align plugins with ops without it)
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.
Let's make it non-public for now
} | ||
|
||
pub fn deserialize<T: DeserializeOwned>(self) -> Result<T, AnyError> { | ||
serde_v8::from_v8(self.scope.unwrap(), self.value.unwrap()) |
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.
Is it expected that this function panics for OpPayload::empty()
? A bit surpising
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.
In production these should never be None
, we only use OpPayload::empty()
for the op-table test, so we don't have to spin up a v8 isolate just to test that
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.
So maybe add #[test]
attribute to OpPayload::empty
let async_responses_size = async_responses.len(); | ||
if async_responses_size == 0 { | ||
return Ok(()); | ||
} | ||
|
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.
Side note: this conditional could potentially be removed after this PR - please see FIXME
below
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.
Yeah, that FIXME
seemed odd to me, since we call JsRuntime.core_js_init()
in JsRuntime::new()
, so I don't think this can ever happen unless you poll the runtime's event-loop while snapshotting ...
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.
Yes, there was a specific condition to trigger the problem and it was because of shared queue, but since shared queue is gone it shouldn't happen anymore.
@AaronO can you please draft a commit message for this PR |
`Deno.core.sharedQueue` was killed in denoland#9843
- Improves op performance. - Handle op-metadata (errors, promise IDs) explicitly in the op-layer vs per op-encoding (aka: out-of-payload). - Remove shared queue & custom "asyncHandlers", all async values are returned in batches via js_recv_cb. - The op-layer should be thought of as simple function calls with little indirection or translation besides the conceptually straightforward serde_v8 bijections. - Preserve concepts of json/bin/min as semantic groups of their inputs/outputs instead of their op-encoding strategy, preserving these groups will also facilitate partial transitions over to v8 Fast API for the "min" and "bin" groups
This is the first PR in a series refactoring the op-layer to be both simpler and faster, it implements ideas shared in #9540
Keep in mind that given the breadth of changes, the intermediate code does not reflect the final intended state.
The bulk of the changes in this PR are scoped to
core/
Key ideas
serde_v8
for a maximally efficient bijection betweenrust <> js
valuesjs_recv_cb
serde_v8
bijectionsjson/bin/min
as semantic groups of their inputs/outputs instead of their op-encoding strategy, preserving these groups will also facilitate partial transitions over to v8 Fast API for the "min" and "bin" groupsChanges
serde_v8
OpFn
sig to ~=(state, payload, optional_buffer) -> serializable
reg_json_sync
or similarserde_v8
tweaks (mainly to relax type-checking and be more permissive)serde_v8
(after which only bytes-support would be unimplemented, intentionally)core.js
to a prior simpler versioncore.js
JsRuntime
tests regarding shared queue or old op-logicresolve_dns
, broken due to[email protected]
not implementingdeserialize_enum
promise_id
out ofOpFn
sig and out ofOpResult
zeroCopy
an optional arg instead of variadicbinOpSync/binOpAsync
?http_bench_json_ops
to follow best practicesserde_json
's handling of map-pairs withundefined
values inserde_v8
OpBuf
,RcOpState
type aliasesserde_v8
fail when deserializing non-utf8 strings (TODO: add failing tests)Follow-up PRs
BufVec
serde_json::Value
andjson!()
)url_parse
metrics_op
since it increases our baseline op-overhead (accounting for nearly 50% of the baseline iirc, after all the other optimizations have been made)op_now
by switching it to a json-op or introducing min-ops (first candidates for v8 Fast API)core/
(similar to https://gist.github.com/AaronO/41803d5fc131417221a78c911cd3c6b0) (✅ Done: perf(core): op_baseline bench #9924)core/README
to reflect new op-layerpromiseTable
performance (obj vs map vs ?)Notes
http_bench_bin_ops
, it will fail due to the runtime assertion of bin-ops requiring a non-null buffer arg. In practice all of deno's bin-ops follow this and this assertion will be removed by the follow-up PR1.
removing the variadicBufVec