Skip to content
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

Merged
merged 34 commits into from
Apr 5, 2021

Conversation

AaronO
Copy link
Contributor

@AaronO AaronO commented Apr 4, 2021

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 of serde_v8 (since it otherwise does a double serializaiton and it's serialized as a loosely typed map, which is very inefficient, doesn't benefit from v8_struct_key optims, etc...)

Goals

  1. More consistent op functions
  2. More performant ops (removing overhead of json!() and serde_json::Value which are loosely typed)
  3. Better typed op code (easier to maintain, catch bugs, etc...)

Changes related to best-practices

  1. Remove wrappers for single args (e.g: { rid })
  2. Remove single-keyed structs to deserialize said wrappers
  3. Specific return values types instead of general Result<Value, AnyError>
  4. Empty returns: Ok(json!({})) => Ok(())
  5. Use rust's unit type for ignored/unused args
  6. Remove most serde_json::* imports and sometimes serde::Deserialize (after dropping wrapper structs)
  7. Remove pseudo-variadic ZeroCopyBufs in JS

Steps

  • runtime/
    • Bulk of modules, with relatively simple changes
    • Modules like net that require introducing new general types
  • op_crates

Notes

  • Yes, this touches a lot of the code
  • This should mostly change rust code, but I've touched JS code to match up the arg-unwrapping
  • Some places that are loosely typed or that return HashMaps still use serde_json::Value
  • This also applies to places where 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, since json!() would convert them to owned types anyway

@AaronO
Copy link
Contributor Author

AaronO commented Apr 5, 2021

This isn't exhaustive and doesn't fix all non-best practices, but fixes most of them.

Most of the remaining uses of serde_json::Value or json!() in ops are either loosely-typed returns or more involved returns (e.g: runtime/net) that should be handled in a follow-up.

@AaronO
Copy link
Contributor Author

AaronO commented Apr 5, 2021

op_crates is now serde_json free, except websocket.

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!({}))`
…elete()

Since it doesn't propagate the null returned from rust and simply has no `return` in the JS function
Copy link
Member

@bartlomieju bartlomieju left a 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

cli/dts/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
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.
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bartlomieju bartlomieju merged commit 2aed322 into denoland:main Apr 5, 2021
@AaronO AaronO deleted the refactor/ops-best-practices branch April 5, 2021 16:42
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Apr 6, 2021
This commit rewrites most of the ops to use "serde_v8" instead
of "json" serialization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants