-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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(ops): first pass at op best practices #10009
refactor(ops): first pass at op best practices #10009
Conversation
Covered most of the files, except those that use extensive use of json!() or those that require a little more care to introduce the correct shared types (e.g: net)
Split rid solo and rid with err patterns
This isn't exhaustive and doesn't fix all non-best practices, but fixes most of them. Most of the remaining uses of |
|
Since serde_v8 never returns undefined
Since the error handling on the JS side depends on destructuring `{err}`, which obviously fails on null. So instead we add a `WebGpuResult::empty()`, for the ops that previously returned `Ok(json!({}))`
…tead of undefined
…elete() Since it doesn't propagate the null returned from rust and simply has no `return` in the JS function
Other serde_json-isms still remain
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.
The PR looks good to me, and if tests are passing I think we're good. Before landing I'll let other take a look
Avoiding allocs of converting those to owned strings (in practice this probably has no impact, since this setup function should be called infrequently)
…null instead of undefined" This reverts commit 4a5798f.
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
This commit rewrites most of the ops to use "serde_v8" instead of "json" serialization.
This PR is a follow-up to #9843 and tweaks most ops across the codebase to follow our best practices for ops.
It's also important to move away from
serde_json::Value
to fully realize the perf gains ofserde_v8
(since it otherwise does a double serializaiton and it's serialized as a loosely typed map, which is very inefficient, doesn't benefit fromv8_struct_key
optims, etc...)Goals
json!()
andserde_json::Value
which are loosely typed)Changes related to best-practices
{ rid }
)Result<Value, AnyError>
Ok(json!({}))
=>Ok(())
serde_json::*
imports and sometimesserde::Deserialize
(after dropping wrapper structs)Steps
runtime/
net
that require introducing new general typesop_crates
Notes
serde_json::Value
json!()
offered some "useful" implicit conversions for references (e.g:&str
,Cow
, ...) to owned types (that can be returned and serialized), to my understanding there was no (performance) upside to returning refs, sincejson!()
would convert them to owned types anyway